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

Bugfix/custom build arguments #601

Merged
merged 2 commits into from
Apr 22, 2019
Merged

Bugfix/custom build arguments #601

merged 2 commits into from
Apr 22, 2019

Conversation

lambdaclan
Copy link
Contributor

@lambdaclan lambdaclan commented Apr 19, 2019 •
edited
Loading

Description

Prior to rez version 2.26, the function call to bind_cli within src/rez/build.py was passing the parser object as an argument which was then used to store the extra args via setattr defined within parse_build_args.py onto so that we can get to it in self.build.

From version 2.26 onwards the passed arguments changed from ArgumentParser to an argument group object in order to display the extra arguments in a separate group for help messages.

The above change created a regression because although the custom build arguments (defined in parse_build_args.py) are visible when calling rez-build --help as an additional group the corresponding environment variables are not created in the build environment because the setattr function is using the wrong object.

This pull request fixes the problem by passing both the group and parser objects to correctly generate the help menu but also ensure the environment variables are set.

Fixes # (issue)
#590, #587

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

Please see #587

How Has This Been Tested?

I have manually tested the changes myself multiple times with various arguments by proving
a series of parse_build_args.py files. Furthermore other members of the community have
tried my changes are have reported them to be working.

I made a custom build with two cherry-picked commits from @lambdaclan and it fixed the issue for me.

Would be fantastic to have a PR for that so that we don't have to deploy a custom version, nothing really bad about it but it is extra maintenance steps.

Cheers,

Thomas

Todos

  • Tests
  • Documentation

@nerdvegas Please advice 🙏

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

@lambdaclan
fix(rez-build): pass both the parser and the group to bind cli
e7aecb4 
Pass both the parser and group object to the bind cli function since
passing only the group is causing a regression where the custom
arguments defined in parse_build_args.py are not properly set
as attributes.

Relates [#587](#587)
@lambdaclan
fix(rez-build): use group for grouping and parser to set attributes
75e1e0f 
Use the group param to enable the grouping during the
display of the help menu. Use the parser param to correctly set the
attributes of the custom arguments that will become environment
variables.

Relates 992e7e9, #587
@nerdvegas nerdvegas merged commit 75e1e0f into AcademySoftwareFoundation:master Apr 22, 2019
@lambdaclan
Copy link
Contributor Author

🎆 First merge, hopefully more to come :)

@lambdaclan lambdaclan deleted the bugfix/custom-build-arguments branch April 6, 2020 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lambdaclan @nerdvegas