gccrs icon indicating copy to clipboard operation
gccrs copied to clipboard

Check for cargo when building rust language

Open P-E-P opened this issue 1 year ago • 15 comments

Prevent rust language from building when cargo is missing.

P-E-P avatar Apr 05 '24 11:04 P-E-P

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 avatar Apr 07 '24 21:04 tschwinge

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 avatar Apr 07 '24 21:04 tschwinge

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.

tschwinge avatar Apr 07 '24 21:04 tschwinge

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 avatar Apr 08 '24 08:04 P-E-P

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?

CohenArthur avatar Apr 10 '24 13:04 CohenArthur

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.

P-E-P avatar Apr 10 '24 18:04 P-E-P

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

tschwinge avatar Apr 12 '24 14:04 tschwinge

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

CohenArthur avatar Apr 12 '24 14:04 CohenArthur

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.

tschwinge avatar Apr 12 '24 15:04 tschwinge

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

CohenArthur avatar Apr 12 '24 15:04 CohenArthur

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

P-E-P avatar May 15 '24 09:05 P-E-P

@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?

CohenArthur avatar Jul 16 '24 11:07 CohenArthur

@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.

P-E-P avatar Jul 17 '24 09:07 P-E-P

@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

CohenArthur avatar Jul 18 '24 12:07 CohenArthur

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

P-E-P avatar Oct 10 '24 12:10 P-E-P