-
Notifications
You must be signed in to change notification settings - Fork 31
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
#93: gprproject/__init__.py, gprproject/gprbuild.py: python builder: implementation of --gpr-opts #94
Conversation
…ilder: implementation of --gpr-opts
@@ -194,6 +201,10 @@ def run(self, args: list[str], **kwargs) -> int: | |||
for key, value in self.variables.items(): | |||
cmd.append(f"-X{key}={value}") | |||
|
|||
if self.gpr_opts is not None: |
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.
This block of code can be moved into the if cmd_name == "gprbuild":
block above. Otherwise, arguments may be incorrectly added to gprinstall, which supports fewer arguments than gprbuild. Based on the parameter description, I assumed that gpr_opts
is intended to be used exclusively for gprbuild.
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.
The gpr_opts
option was already in the code base but not implemented. It is not specific to gprbuild, but should be available to gprbuild, gprclean and gprinstall. In case of any limitation on the higher level building infrastructure, having this option available might help the developer.
@@ -277,6 +288,7 @@ def save(self): | |||
"symcc": self.symcc, | |||
"prefix": self.prefix, | |||
"gpr_paths": self.gpr_paths, | |||
"gpr_opts": self.gpr_opts, |
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 response to https://github.com/AdaCore/gnatcoll-core/pull/94/files#r1881804015:
To make it work properly, this line needs to be removed. Each time the build command is run, its environment and provided arguments are saved in a JSON file for reuse by the clean, uninstall, and install commands. You can search for "load" in gprproject/init.py
for reference. If we store gpr_opts
arguments here, they may not be valid for the other tools. For instance:
$ gnatcoll_core.gpr.py build --gpr-opts="-O0"
$ gnatcoll_core.gpr.py install <<---- raises gprinstall: unrecognized option '-O0'
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.
Sure I understand, but in my case the argument needs to be passed to all the gpr tools.
So may be --gpr-opts
for arguments to be passed to all gpr tools, and then
--gprbuild-opts
,--gprclean-opts
,--gprinstall-opts
for argument to specific gpr tools.
If you are interested to get same I can initiate a new PR in that direction, in any case I would close this PR.
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.
This is a good solution! I'll review your new PR if you plan to create one.
@@ -70,6 +71,7 @@ def __init__( | |||
:param jobs: level of parallelism for gpr tools that support it | |||
:param gnatcov: if True add gnatcov instrumentation | |||
:param symcc: if True add symcc instrumentation | |||
:param gpr_opts: arguments passed to gprbuild |
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.
Could you update to something like: "arguments passed to gpr tools"
This PR will be closed, and a new PR initiated to fix #93 would be evaluated for proposal, with the following scope;
|
Right. Could you explain the context of this PR to us, if possible? It may help us find a better solution. For instance, we could consider making updates at the GPR tools level. |
In our case, as we needed arguments to be passed to all the gpr tools (dealing with multiple Ada runtime context), we implemented this feature using the existing Since then we implemented a Seems a bug has been fixed in GNAT-FSF-14, we might not need to use the |
See #93