-
Notifications
You must be signed in to change notification settings - Fork 217
support for injecting additional toolchainopts (WIP) #2368
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
base: develop
Are you sure you want to change the base?
support for injecting additional toolchainopts (WIP) #2368
Conversation
| param, value = spec.split('=') | ||
| toolchainopts.update({param: value}) | ||
| build_specs.update({'toolchainopts': toolchainopts}) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case someone uses --extra-toolchains without arguments, is this caught somewhere?
If not, only set try_to_generate if toolchainopts is actually non-empty, and don't call build_specs.update if it is empty.
Or perhaps error out if it is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for looking into this
if it is empty the options parser will error out saying that option it requires an argument
but if it is malformed, that's another issue, for instance if there is no '=' the split above will fail with no more values to unpack...
the correct usage allows something like --extra-toolchainopts=usempi=True,openmp=True,optarch="Intel:-O2;GCC=-O3", maybe this should be made clearer somewhere?
|
@migueldiascosta Needs a sync with I think it makes sense to rework this a bit to make it use the |
this is mostly because
--try-amenddoes not support dictionaries, and supporting it for arbitrary easyconfig parameters would require defining a new (too convoluted?) argument syntaximprovements/alternatives?