Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-623 build: Various build improvements #13615

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Conversation

brianjmurrell
Copy link
Contributor

@brianjmurrell brianjmurrell commented Jan 16, 2024

Add get_base_branch utility to walk the commit history backwards until
a base branch is found.

Update packaging:

  • use mock's ccache plugin.
  • use new get_base_branch utility from the packaging update to add the
    base branch to the environment in BASE_BRANCH_NAME for other stages in
    the pipeilne to be able to use.

Use pragmasToEnv() rather than a bigger script that does the same thing
to conserve Jenkinsfile pipeline {} size.

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
    • Hardware testing was skipped as it adds no value to testing the changes
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate watchers.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

Sorry, something went wrong.

@brianjmurrell brianjmurrell self-assigned this Jan 16, 2024
Copy link

github-actions bot commented Jan 16, 2024

Bug-tracker data:
Errors are component not formatted correctly,Ticket number prefix incorrect,PR title is malformatted. See https://daosio.atlassian.net/wiki/spaces/DC/pages/11133911069/Commit+Comments,Unable to load ticket data
https://daosio.atlassian.net/browse/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - the new shellcheck checker relies on shell scripts have a .sh extension.

Copy link
Contributor Author

@brianjmurrell brianjmurrell Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm. That's less than optimal. Typically [shell] scripts that you want to be regularly executable commands have a shebang at the top of them and don't typically have a [.sh] extension on them and scripts you want to be libraries or otherwise executed with a specific interpreter don't have a shebang but do have a [.sh] extension on the end.

I.e.:
https://google.github.io/styleguide/shellguide.html#s2.1-file-extensions
https://www.talisman.org/~erlkonig/documents/commandname-extensions-considered-harmful/
as just a couple of references to the practice.

As an alternative file could be used to determine which linter to use on a file. But I think you told me that we lint the entire code base now, on every commit and not just the files in the commit, yes? I would suppose running file on every file in the tree, on every commit might be onerous.

$ time bash -c 'git ls-files | xargs file >/dev/null'

real    0m6.215s
user    0m6.007s
sys     0m0.130s

I wonder how keeping a cache (i.e. a mapping of filename to file output) of the file results and only running file on new/changed files (and updating the cache with the results) would help that time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old shellcheck did that - but as we have a lot of scripts which aren't executable and don't have a shebang that also missed a bunch of our code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah. If they are not executable and don't have a shebang, those should have a .sh extension. If we fixed that, we'd be all good then it sound like, yes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd want CI to check for all files with a .sh extension and all files that "file" identifies as a shell script? That would be ideal yes, but I don't think it's what we do today.

I think most of the discussion was in #13360 and I remember being surprised that we were missing files with the old system.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a clean checkout "find . -type f -exec file '{}' ; | grep "shell script" | grep -v ".sh"" shows 18 commands, three of which are in vendor and two are .in files which are renamed in #13076 and should not be checked as they're not shell.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13615/4/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13615/4/execution/node/748/log

@brianjmurrell brianjmurrell force-pushed the bmurrell/get_base_branch branch from 53969c4 to e5e3e58 Compare January 17, 2024 22:30
@brianjmurrell brianjmurrell force-pushed the bmurrell/get_base_branch branch from 80355c6 to 41768f6 Compare January 30, 2024 15:50
@brianjmurrell brianjmurrell changed the title DAOS-623 build: Add get_base_branch utility DAOS-623 build: Various build improvements Jan 30, 2024
Add get_base_branch utility to walk the commit history backwards until
a base branch is found.

Update packaging:
- use mock's ccache plugin.
- use new get_base_branch utility from the packaging update to add the
base branch to the environment in BASE_BRANCH_NAME for other stages in
the pipeilne to be able to use.

Mount /scratch in mock container for ccache and mock bootstrap caching.

Add mock group docker run argument to be able to create
/var/cache/mock/$CHROOT_NAME-bootstrap/.

Use pragmasToEnv() rather than a bigger script that does the same thing
to conserve Jenkinsfile pipeline {} size.

Skip-func-test-el9: false
Skip-func-test-leap15: false
Skip-func-hw-test: true
Skip-PR-comments: true
Test-tag: always_passes

Required-githooks: true

Signed-off-by: Brian J. Murrell <brian.murrell@intel.com>
@brianjmurrell brianjmurrell force-pushed the bmurrell/get_base_branch branch from 701ea66 to b65ddba Compare January 30, 2024 19:30
@ashleypittman ashleypittman self-requested a review January 30, 2024 19:39
@brianjmurrell brianjmurrell marked this pull request as ready for review January 31, 2024 00:56
@brianjmurrell brianjmurrell requested a review from a team as a code owner January 31, 2024 00:56
JohnMalmberg
JohnMalmberg previously approved these changes Jan 31, 2024
@brianjmurrell brianjmurrell added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jan 31, 2024
Jenkinsfile Outdated Show resolved Hide resolved
utils/rpms/packaging/get_base_branch Outdated Show resolved Hide resolved
Comment on lines 12 to 18
for base in "${all_bases[@]}"; do
git rev-parse --verify "$ORIGIN/$base" &> /dev/null || continue
commits_ahead=$(git log --oneline "$ORIGIN/$base..HEAD" | wc -l)
if [ "$min_diff" -eq -1 ] || [ "$min_diff" -gt "$commits_ahead" ]; then
TARGET="$base"
min_diff=$commits_ahead
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, checking commits_behind like find_base.sh does is only for edge cases (if even useful at all). Could we either (A) update find_base.sh to be in-sync or (B) share the function?

Also, is utils/rpms/packaging the right place for this, if only called from the Jenkinsfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, checking commits_behind like find_base.sh does is only for edge cases (if even useful at all).

Right, which is why I omitted that in this implementation.

Could we either (A) update find_base.sh to be in-sync or (B) share the function?

My original intent was to share/genericize the function but you seemed to want to keep the commits_behind functionality in find_base.sh and I didn't really see the need for it here. Indeed, it nags at me that it might make finding the true base (where we disregard intermediate branches such as features/*) actually less accurate.

Also, is utils/rpms/packaging the right place for this, if only called from the Jenkinsfile?

It seems odd, yes, but this functionality is used across projects (daos and all of the dependency packaging projects as it's used in the dependency packaging pipelines) and packaging/ is the common denominator there -- without inventing yet another cross-project library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I'd rather give up commits_behind in find_base.sh than have two slightly different versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this functionality is used across projects

It is currently, or it will be after landed?

Copy link
Contributor Author

@brianjmurrell brianjmurrell Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few moving parts that are working towards this, but it will be used by all project pipelines that build RPMs (hence the association with packaging/.

I would dare say, we could make this a pipeline-lib function instead of a shell script in packaging/ but then we'd lose the ability for the githooks to be able to [re-]use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's better for this function to be in daos instead of pipeline-lib - for re-use purposes as well as ease-of-updating.

The question of "where do I put a script that is used for CI, hooks, etc." has come up before, and I'm not sure where the best place is to put them. I don't want to block on this, so I'm okay with what you have and we can revisit later if/when we start re-using this function elsewhere

Jenkinsfile Outdated
@@ -329,13 +329,13 @@ pipeline {
pragmasToEnv()
}
}
stage('Determine Base Branch') {
stage('Determine Landing Branch') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Landing" branch might be misleading too. If someone opens a PR to be merged into "foo", then "foo" is the immediate landing branch.

Copy link
Contributor Author

@brianjmurrell brianjmurrell Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where foo is based on what? Just so that we are using the same nomenclature, the branch against which a PR is created is called the base branch and the branch that will ultimately result in the change being included in a DAOS release is what I am referring to as the landing branch here.

I'm specifically not looking for the base branch. For example, if somebody opens a PR on a branch called foo with a base branch of master and then opens a PR on a branch named bar against the branch foo, foo is the base branch of bar but master is the landing branch of it. It's that latter, landing branch name that I am looking for.

Would release branch be a more suitable name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The devil is in the nuance :) I think "release" might be the most appropriate.

daltonbohning
daltonbohning previously approved these changes Feb 5, 2024
DRY some near duplication.

Change variable name to be more consistent with Jenkins variables.

Skip-func-test-el9: false
Skip-func-test-leap15: false
Skip-func-hw-test: true
Skip-PR-comments: true
Test-tag: always_passes

Required-githooks: true

Signed-off-by: Brian J. Murrell <brian.murrell@intel.com>
Skip-func-test-el9: false
Skip-func-test-leap15: false
Skip-func-hw-test: true
Skip-PR-comments: true
Test-tag: always_passes

Required-githooks: true

Signed-off-by: Brian J. Murrell <brian.murrell@intel.com>
…ranch

this-is-Signed-off-by: Brian J. Murrell <brian.murrell@intel.com>
Skip-func-test-el9: false
Skip-func-test-leap15: false
Skip-func-hw-test: true
Skip-PR-comments: true
Test-tag: always_passes
@brianjmurrell brianjmurrell force-pushed the bmurrell/get_base_branch branch from acc05e5 to bead334 Compare February 7, 2024 21:58
@brianjmurrell brianjmurrell requested review from daltonbohning and removed request for a team February 7, 2024 21:58
@brianjmurrell
Copy link
Contributor Author

This one is ready for [re-]review please and thanks, in advance.

Comment on lines +328 to +332
stage('Get Commit Message') {
steps {
pragmasToEnv()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly a reminder for myself that when I get the auto-test-tag magic working, we might need to rethink this a bit - or I rethink my approach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should have any impact. This is just an optimization to store the commit pragmas into an env. variable rather than everywhere wanting to examine them running a git command. Ultimately cachedCommitPragma() reads them from the env. var. if it exists and falls back to a git command if not.

And this use of pragmasToEnv() isn't really a change in behaviour. It's just a more code efficient way of what was being done before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my current version, I replace

sh(script: 'git show -s --format=%B',
                                            returnStdout: true).trim()

With a call to get_commit_message.py.

So now the question is how to call get_commit_message.py before pragmasToEnv(). But we can look at that later. The work is still some ways off

Jenkinsfile Show resolved Hide resolved
stage('Determine Release Branch') {
steps {
script {
env.RELEASE_BRANCH = releaseBranch()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does releaseBranch call get_release_branch?

Copy link
Contributor Author

@brianjmurrell brianjmurrell Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if double posted, but it seems GH lost some of my comments...

This
daos:/Jenkinsfile
calls
pipeline-lib:/vars/releaseBranch.groovy
which calls
daos:/utils/rpms/packaging/get_release_branch

IMO that's messy to bounce back and forth between repos

min_diff=$commits_diff
fi
done
TARGET="$ORIGIN/$(utils/rpms/packaging/get_release_branch "feature/cat_recovery feature/multiprovider")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm almost tempted to say we don't need to look at the feature branches, since as you pointed out before, they should be up-to-date with master anyway.

But I think we could handle that later after consulting with other relevant parties

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. We actually have lots of that going on because it's a necessary evil we need to do to minimize the amount of code we add to Jenkinsfile's pipeline {} block due to the design flaw of how Jenkins renders that into Java.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my limited understanding of the issue, it's a limit on the static pipeline size. But function calls should by dynamic. So I would think something like this should work, even putting groovy scripts in the daos repo

load("my_func.groovy").foo()

Anyway, that's a much larger problem to address so doesn't need to block this PR.

@brianjmurrell brianjmurrell requested a review from a team February 12, 2024 19:48
@phender phender merged commit 99902ff into master Feb 15, 2024
48 checks passed
@phender phender deleted the bmurrell/get_base_branch branch February 15, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed.
Development

Successfully merging this pull request may close these issues.

None yet

6 participants