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.

Sign up for GitHub

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

Jump to bottom

DAOS-623 build: Use AddMethod for adding methods to environments. #10654

Merged
merged 8 commits into from
Nov 3, 2022
Merged

DAOS-623 build: Use AddMethod for adding methods to environments. #10654

merged 8 commits into from
Nov 3, 2022

Conversation

ashleypittman
Copy link
Contributor

Signed-off-by: Ashley Pittman [email protected]

@github-actions
Copy link

Bug-tracker data:
Ticket title is 'Generic ticket for minor code cleanup and improvement'
Status is 'Resolved'
https://daosio.atlassian.net/browse/DAOS-623

daosbuild1
daosbuild1 reviewed Oct 24, 2022
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

daosbuild1
daosbuild1 reviewed Oct 24, 2022
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-10654/2/execution/node/639/log

@ashleypittman
Fix filenames and typo.
cd376e4 
Signed-off-by: Ashley Pittman <[email protected]>
daosbuild1
daosbuild1 reviewed Oct 24, 2022
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

ashleypittman
ashleypittman commented Oct 24, 2022
View reviewed changes
site_scons/daos_build.py
Comment on lines +237 to +242
def setup(env):
"""Add daos specific methods to environment"""
env.AddMethod(_add_build_rpath, 'd_add_build_rpath')
env.AddMethod(_configure_mpi, 'd_configure_mpi')
env.AddMethod(_run_command, 'd_run_command')
env.AddMethod(_add_rpaths, 'd_add_rpaths')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the real change in this PR, the rest is just fluff.

  1. Is this worthwhile?
  2. do we want a common 'd_' prefix for all our methods to make it clear they're non-standard.
  3. Should we copy the CamelCase naming of existing methods?
  4. Assuming this is worthwhile should we do it for all daos_build functions (which will touch more code than this PR).

Copy link
Contributor

@jolivier23 jolivier23 Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For point 2, I think we already have other precedent where we add stuff and don't name it specially. For instance, Configure CheckFlag and CheckFlagCC. I think it would be fine to do something like AddBuildRPATH. For point 4, possibly. But I wouldn't do that in a single PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would mean that we'd replace daos_build.library() with env.Library(), yet there is already a method with that name, the one that we're wrapping/

Copy link
Contributor

Choose a reason for hiding this comment

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

You could do something like DAOSLibrary, I suppose

@ashleypittman ashleypittman marked this pull request as ready for review October 24, 2022 19:59
@ashleypittman ashleypittman requested review from a team as code owners October 24, 2022 19:59
jolivier23
jolivier23 previously approved these changes Oct 24, 2022
View reviewed changes
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-10654/3/execution/node/1131/log

@ashleypittman
Fix typo.
d0289d4 
Skip-func-hw-test: true
Skip-func-test: true
Quick-Functional: true
Test-tag: dfusedaosbuild
@ashleypittman
Merge remote-tracking branch 'origin/master' into amd/scons-methods
4be9ce5
daosbuild1
daosbuild1 reviewed Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

daosbuild1
daosbuild1 reviewed Oct 27, 2022
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

daosbuild1
daosbuild1 reviewed Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-10654/6/execution/node/878/log

daltonbohning
daltonbohning approved these changes Nov 2, 2022
View reviewed changes
SConstruct Show resolved Hide resolved
utils/sl/fake_scons/SCons/Script/__init__.py Show resolved Hide resolved
@ashleypittman ashleypittman requested a review from a team November 2, 2022 18:56
@jolivier23 jolivier23 merged commit 1f456a7 into master Nov 3, 2022
@jolivier23 jolivier23 deleted the amd/scons-methods branch November 3, 2022 23:04
ashleypittman added a commit that referenced this pull request Nov 4, 2022
@ashleypittman
DAOS-623 build: Use AddMethod for adding methods to environments. (#1…
739c76f 
…0654)

* Apply change to compiler_setup.

Signed-off-by: Ashley Pittman <[email protected]>
wangzhaorong-cestc pushed a commit to wangzhaorong-cestc/daos that referenced this pull request Nov 20, 2022
@ashleypittman @wangzhaorong-cestc
DAOS-623 build: Use AddMethod for adding methods to environments. (da…
8f6bf44 
…os-stack#10654)

* Apply change to compiler_setup.

Signed-off-by: Ashley Pittman <[email protected]>
@mjmac mjmac mentioned this pull request Dec 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daosbuild1 daosbuild1 daosbuild1 left review comments

@daltonbohning daltonbohning daltonbohning approved these changes

@jolivier23 jolivier23 jolivier23 approved these changes

@soumagne soumagne Awaiting requested review from soumagne

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ashleypittman @daosbuild1 @jolivier23 @daltonbohning