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

Set tool prefix for spawning GNU tool processes #368

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

jld01
Copy link
Contributor

@jld01 jld01 commented Apr 15, 2023

Allows the GNU tool prefix to be specified by a CDT build variable.

Modifies the Cross GCC toolchain description to provide the GNU tool prefix.

Part of #361

Copy link
Member

@Torbjorn-Svensson Torbjorn-Svensson left a comment

Choose a reason for hiding this comment

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

Thanks for working on this John!

I see that you are handling the prefix here and I think that the path is handled with the environment provider, but would it make sense to also expose the path using a build variable?

@@ -32,7 +34,7 @@
targetTool="cdt.managedbuild.tool.gnu.cross.c.linker;cdt.managedbuild.tool.gnu.cross.cpp.linker;cdt.managedbuild.tool.gnu.archiver">
<targetPlatform
archList="all"
binaryParser="org.eclipse.cdt.core.ELF"
binaryParser="org.eclipse.cdt.core.GNU_ELF"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the binary parser attribute is written out in the .cproject file of every created project.
While this in general would be good, for this particular change, it looks like all the existing projects will be stuck on org.eclipse.cdt.core.ELF until a manual change is done. Do you have any idea how to automatically migrate existing projects to the org.eclipse.cdt.core.GNU_ELF parser instead?

Copy link
Contributor Author

@jld01 jld01 Apr 16, 2023

Choose a reason for hiding this comment

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

Will think about this. Perhaps as a separate issue. It is easy to switch the active binary parser in the CDT project settings UI.

Do you think there could be any reason why someone would NOT wish to update their parser? I think it would be wise to prompt for confirmation if we decide to proceed.

Cc: @jonahgraham

Copy link
Member

Choose a reason for hiding this comment

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

Right now, I do not see any reason why you would not want that.

Maybe we should invert the question and rather ask us why anyone would like to use the org.eclipse.cdt.core.ELF parser in the first place and if we have no clear answer to that, perhaps the org.eclipse.cdt.core.ELF and org.eclipse.cdt.core.GNU_ELF should be the same parser from now on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are, of course, non-GNU toolchains that generate ELF executable files. At present, the existence of the ELF parser and the GNU_ELF parser allows common code to determine whether use of GNU tools such as objcopy should be attempted. In theory, an extender could integrate equivalent non-GNU tools.

@jld01
Copy link
Contributor Author

jld01 commented Apr 16, 2023

Thanks for working on this John!

would it make sense to also expose the path using a build variable?

@Torbjorn-Svensson, a toolchain can end up on the PATH of a build configuration via many routes. The CDT builder just assumes that the ITool commands have made it onto the PATH and I think we can do the same for the various GNU binutils. It seems unlikely that the user will wish to access these tools at a different location to the compiler and linker. Even, if they do, they can just prepend the folder to the PATH.

Allows the GNU tool prefix to be specified by a CDT build variable.

Modifies the Cross GCC toolchain description to provide the GNU tool
prefix.

Part of eclipse-cdt#361
@jld01 jld01 force-pushed the gnu-tool-prefix-var branch from c3daec5 to d06f55b Compare April 17, 2023 08:29
Copy link
Member

@Torbjorn-Svensson Torbjorn-Svensson left a comment

Choose a reason for hiding this comment

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

Other than 2 more places of the "null" part being on the left side (that I do not really care about), I think this looks good.

@jld01 jld01 merged commit 0d9fa84 into eclipse-cdt:main Apr 17, 2023
@jld01
Copy link
Contributor Author

jld01 commented Apr 17, 2023

Thank you for the helpful reviews, @Torbjorn-Svensson. Perhaps we can discuss automatic update of the binary parser in legacy projects on the next CDT project call.

@jld01 jld01 added this to the 11.2.0 milestone Apr 17, 2023
@Torbjorn-Svensson
Copy link
Member

Thank you for the helpful reviews, @Torbjorn-Svensson. Perhaps we can discuss automatic update of the binary parser in legacy projects on the next CDT project call.

Your welcome John! :)

@jonahgraham, can you add this item to the next monthly call?

@jld01 jld01 deleted the gnu-tool-prefix-var branch April 28, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants