Bug 937470 – cargo.eclass: RUSTFLAGS merging broken (dev-util/cargo-nextest-0.9.72: fails to compile because it can't merge rustflags)

• Product: Gentoo Linux
• Status: RESOLVED FIXED

[Description Benjamin Neff 2024-08-07 00:40:24 UTC]

The project brings it's own .cargo/config.toml where it definies it's own build.rustflags, which is defined as an array, but the generated work/cargo_home/config.toml also has a build.rustflags config, which is defined as a string, and cargo fails to merge these.

The best solution would probably be to make the generated rustflags from the cargo.eclass also an array (so maybe this should be a bug for cargo.eclass?), then they are properly merged and all flags from both configs are present. But that would then break if a project has rustflags which are a string instead of an array (with the same error as now, as it can't merge string and array, no matter in which direction), which I don't know if there are existing ebuilds where this is the case. But when both are a string, the one from the project wins, and the rustflags from the cargo.eclass are completely ignored. So maybe if an ebuild for such a project already exists, which has it's own rustflags as a string, that might already be a problem, as it overwrites all other rustflags?

Otherwise the rustflags from .cargo/config.toml would need to be patched out (so it doesn't try to merge/overwrite), and then manually merged to the generated config. But that feels like a hack.

Reproducible: Always

Steps to Reproduce:
1. emerge dev-util/cargo-nextest
Actual Results:  
error: could not load Cargo configuration

Caused by:
  failed to merge configuration at `/tmp/portage/dev-util/cargo-nextest-0.9.72/work/cargo_home/config.toml`

Caused by:
  failed to merge key `build` between /tmp/portage/dev-util/cargo-nextest-0.9.72/work/nextest-cargo-nextest-0.9.72/.cargo/config.toml and /tmp/portage/dev-util/cargo-nextest-0.9.72/work/cargo_home/config.toml

Caused by:
  failed to merge key `rustflags` between /tmp/portage/dev-util/cargo-nextest-0.9.72/work/nextest-cargo-nextest-0.9.72/.cargo/config.toml and /tmp/portage/dev-util/cargo-nextest-0.9.72/work/cargo_home/config.toml

Caused by:
  failed to merge config value from `/tmp/portage/dev-util/cargo-nextest-0.9.72/work/cargo_home/config.toml` into `/tmp/portage/dev-util/cargo-nextest-0.9.72/work/nextest-cargo-nextest-0.9.72/.cargo/config.toml`: expected array, but found string

Expected Results:  
It should compile

[Comment 1 Benjamin Neff 2024-08-07 00:41:19 UTC]

Created attachment 899323 [details]
emerge --info

[Comment 2 Benjamin Neff 2024-08-07 00:43:17 UTC]

Created attachment 899324 [details]
build.log

[Comment 3 James Le Cuirot 2024-08-07 06:35:47 UTC]

Thanks, already aware of another instance of this. I was able to make it an array easily enough last night, but it seemed like it was then just ignoring our flags. Cargo has is really bad at this all round, but I'll try to work something out. Got one or two more ideas.

[Comment 4 Benjamin Neff 2024-08-07 11:39:27 UTC]

> but it seemed like it was then just ignoring our flags.

Interesting, when I edited the build.rustflags in .cargo/config.toml manually to be an array (split at spaces), it compiled just fine, and it looked like it used all of them from both files.

[Comment 5 James Le Cuirot 2024-08-07 14:20:00 UTC]

(In reply to Benjamin Neff from comment #4)
> > but it seemed like it was then just ignoring our flags.
> 
> Interesting, when I edited the build.rustflags in .cargo/config.toml
> manually to be an array (split at spaces), it compiled just fine, and it
> looked like it used all of them from both files.

Yeah, I was testing a different package. It depends on whether the project puts its flags under target-specific config or generic [build] config. I've put together some changes, just getting some feedback now.

[Comment 6 James Le Cuirot 2024-08-07 16:08:41 UTC]

Fix proposed here:
https://marc.info/?l=gentoo-dev&m=172304650421630&w=2

[Comment 7 tt_1 2024-08-07 16:15:41 UTC]

maybe I'm dumb, but where is the patch on the mailing list? can you maybe add it here as well, I'd like to test it and give you feedback.

[Comment 8 James Le Cuirot 2024-08-07 16:35:16 UTC]

They're in the following messages of that thread. You can also get them here.
https://github.com/gentoo/gentoo/compare/master...chewi:gentoo:cargo-rustflags

[Comment 9 Michał Górny 2024-08-07 17:37:44 UTC]

*** Bug 937520 has been marked as a duplicate of this bug. ***

[Comment 10 tt_1 2024-08-08 08:39:55 UTC]

thank you, I tested it with cross compile and it now works again (if I set up the linker additionally) - so the cross compile ability is back to its old state. 

I see one problem: the changes pull in app-misc/yq, which lacks keywords, and app-misc/jq, which lacks stable keywords - this should lead to destabilizing of packages on non amd64/arm64/x86 for librsvg and friends. 

so maybe app-misc/yq should be keyworded & stabilized before the changes are commited?

[Comment 11 Larry the Git Cow 2024-08-08 09:00:50 UTC]

The bug has been closed via the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=27d469a2114b4ad0b3e682854c50c806753eb472

commit 27d469a2114b4ad0b3e682854c50c806753eb472
Author:     James Le Cuirot <[e-mail]>
AuthorDate: 2024-08-07 14:27:36 +0000
Commit:     James Le Cuirot <[e-mail]>
CommitDate: 2024-08-08 09:00:08 +0000

    cargo.eclass: Change RUSTFLAGS approach following recent build failures
    
    Cargo turned out to be even worse at handling flags than I thought.
    Target-specific flags set by projects were overriding our own, and Cargo
    was bailing out when faced with merging a string of flags with an array
    of flags.
    
    After weighing up the poor set of options, I've found that it is better
    to always set flags via a target-specific environment variable for
    reasons set out in comments added here. This approach was previously
    just used for cross-compiling, but now we do it unconditionally.
    
    It has the downside of overriding generic [build] flags set by projects,
    but these were already being overridden by users and ebuilds setting
    RUSTFLAGS themselves.
    
    Closes: https://bugs.gentoo.org/937453
    Closes: https://bugs.gentoo.org/937470
    Signed-off-by: James Le Cuirot <[e-mail]>

 eclass/cargo.eclass | 65 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

[Comment 12 James Le Cuirot 2024-08-08 09:04:34 UTC]

(In reply to tt_1 from comment #10)
> so maybe app-misc/yq should be keyworded & stabilized before the changes are
> commited?

I'm aware, but we're discussing alternative approaches on the mailing list. Python has a built-in toml parser, so I may just use that.

In the meantime, I've merged the first commit, which fixes the main issue. The remaining issue is less likely to cause real problems.

[Comment 13 Sam James 2024-08-08 09:04:55 UTC]

(In reply to tt_1 from comment #10)
> thank you, I tested it with cross compile and it now works again (if I set
> up the linker additionally) - so the cross compile ability is back to its
> old state. 
> 
> I see one problem: the changes pull in app-misc/yq, which lacks keywords,
> and app-misc/jq, which lacks stable keywords - this should lead to
> destabilizing of packages on non amd64/arm64/x86 for librsvg and friends. 
> 
> so maybe app-misc/yq should be keyworded & stabilized before the changes are
> commited?

That's unavoidable, because the tree would be broken (CI would fail) and the changes wouldn't reach users. So, it makes sense to say it in the sense of "you may want to do that to get on with the process", but not in the sense of "it would lead to loads of stuff being destabled" because that's obviously not going to happen.

[Comment 14 tt_1 2024-08-08 09:34:06 UTC]

I guess with the recent changes, the compile of dev-util/cbindgen is broken on musl. Will open a new bug later, once I had enough time to understand it better.

[Comment 15 Michał Górny 2024-08-08 10:52:30 UTC]

*** Bug 937555 has been marked as a duplicate of this bug. ***