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

Check for cargo when building rust language #2942

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Apr 5, 2024

Prevent rust language from building when cargo is missing.

@P-E-P P-E-P added the upstream Issue regarding upstreaming gccrs into GCC label Apr 5, 2024
@P-E-P P-E-P self-assigned this Apr 5, 2024
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@tschwinge
Copy link
Member

Is it expected that GCC Rust build and test / build-and-check-gcc-48 failed here: configure: error: Cargo is required to build rust -- per the Install Deps step, Rust/Cargo is being installed? Is this hopefully just some kind of scripting issue?

@tschwinge tschwinge linked an issue Apr 7, 2024 that may be closed by this pull request
@tschwinge
Copy link
Member

Per a quick look, these changes look like (the first part of) how I think they should look like, thanks! In case there are requests for changes, I suggest to first get this reviewed on [email protected] and committed upstream, before pushing to GCC/Rust (here).

@tschwinge
Copy link
Member

And then ("the next part") we should add a check that the object files that Cargo produces for a simple (dummy) crate can actually be linked into the GCC compiler we're about to build. This is to detect early cases like described in #2898 "cargo should build for the host system", or #2899 "Pass configuration flags to cargo", and still be able to react (disable Rust language, or sensible error message, as applicable), instead of later running into a build error.

@P-E-P
Copy link
Member Author

P-E-P commented Apr 8, 2024

And then ("the next part") we should add a check that the object files that Cargo produces for a simple (dummy) crate can actually be linked into the GCC compiler we're about to build. This is to detect early cases like described in #2898 "cargo should build for the host system", or #2899 "Pass configuration flags to cargo", and still be able to react (disable Rust language, or sensible error message, as applicable), instead of later running into a build error.

In fact, I've been hesitating for a long time to also check which edition of rust can be compiled but @CohenArthur convinced me to keep it simple.

@P-E-P P-E-P force-pushed the check-for-cargo-top-level branch 4 times, most recently from c60ce2b to 9876f36 Compare April 8, 2024 11:56
@CohenArthur
Copy link
Member

Per a quick look, these changes look like (the first part of) how I think they should look like, thanks! In case there are requests for changes, I suggest to first get this reviewed on [email protected] and committed upstream, before pushing to GCC/Rust (here).

patch has been reviewed and approved by Richard Biener, so I think we can merge it here as well. @P-E-P do you want me to take care of the CI issue?

@P-E-P
Copy link
Member Author

P-E-P commented Apr 10, 2024

do you want me to take care of the CI issue?

@CohenArthur Yes please. I won't be able to work on it for a while.

@tschwinge
Copy link
Member

So, are you going to git push the patch into GCC upstream?

@CohenArthur
Copy link
Member

@tschwinge I was thinking of pushing it as part of our next upstream? would you rather it was pushed beforehand?

@tschwinge
Copy link
Member

tschwinge commented Apr 12, 2024

I prefer these generic non-trivial pieces to be integrated incrementally in GCC upstream: one after the other (in a logically meaningful order) instead of in bulk, so that there's some time for testing/stabilization between the individual parts.

@CohenArthur
Copy link
Member

alright :) that commit is good to upstream then I think. I'll fix the CI so we can merge it here

@P-E-P
Copy link
Member Author

P-E-P commented May 15, 2024

@tschwinge I'm a bit lost, should we merge this ? IIRC we already sent this upstream, so we should technically close this PR right ?

@CohenArthur
Copy link
Member

@tschwinge I'm a bit lost, should we merge this ? IIRC we already sent this upstream, so we should technically close this PR right ?

I think yes, we'll get the content from upstream when we update our fork. or we can also merge it if the commits match exactly so that it gets skipped once we rebase on gcc's trunk. @P-E-P can you check if there are any differences between what was merged on trunk and this?

@P-E-P
Copy link
Member Author

P-E-P commented Jul 17, 2024

@tschwinge I'm a bit lost, should we merge this ? IIRC we already sent this upstream, so we should technically close this PR right ?

I think yes, we'll get the content from upstream when we update our fork. or we can also merge it if the commits match exactly so that it gets skipped once we rebase on gcc's trunk. @P-E-P can you check if there are any differences between what was merged on trunk and this?

There are some differences, this explains my confusion. This PR adds some documentation and modifies the CI, upstream content only has the cargo checking in the autoconf configuration file.

@CohenArthur
Copy link
Member

@P-E-P then we should merge this too, but make sure that the commit which touches the build system is exactly the same here as it was pushed on trunk

Prevent rust language from building when cargo is
missing.

config/ChangeLog:

	* acx.m4: Add a macro to check for rust
	components.

ChangeLog:

	* configure: Regenerate.
	* configure.ac: Emit an error message when cargo
	is missing.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
Cargo is temporarily required in order to build some parts of GCC rust
frontend. This requirement was not mentioned in the documentation.

gcc/ChangeLog:

	* doc/install.texi: Mention cargo requirement.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P
Copy link
Member Author

P-E-P commented Oct 10, 2024

I'll wait for #2979 to be merged first.


In order to build GCCRS you need a working rust compiler, for now we
are using cargo and rustc but in the future GCC will be able to compile
it's own rust components and recompile itself with those components
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it's own rust components and recompile itself with those components
its own rust components and recompile itself with those components

@@ -313,6 +313,13 @@ documentation including the target @code{SYSTEM} definition module.
If Python3 is unavailable Modula-2 documentation will include a target
independent version of the SYSTEM modules.

@item @anchor{GCCRS-prerequisite}Rust

In order to build GCCRS you need a working rust compiler, for now we
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In order to build GCCRS you need a working rust compiler, for now we
In order to build GCCRS, you need a working Rust compiler. For now, we

@P-E-P P-E-P closed this Oct 24, 2024
@P-E-P P-E-P reopened this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream Issue regarding upstreaming gccrs into GCC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checking that cargo exists when building gccrs
4 participants