Make target pointer width in target json an integer

[WaffleLapkin]

r? Noratrieb
cc @RalfJung (https://github.com/rust-lang/rust/pull/142352/files#r2230380120)

try-job: x86_64-rust-for-linux

[rustbot]

Noratrieb is not on the review rotation at the moment.
They may take a while to respond.

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[Noratrieb]

I'm unsure if it's worth breaking everyone over such a minor improvement.. maybe? maybe not?

[Noratrieb]

I'm inclined to say yes, but it would probably be good to get this merged today so it can be in the same nightly as the other breaking change instead of two consecutive nightlies (it would also be fine to do that, but less cool)

[RalfJung]

I'm unsure if it's worth breaking everyone over such a minor improvement.. maybe? maybe not?

We already did that with #144443... the current state is just inconsistent.

But, no strong opinion.

[WaffleLapkin]

@Noratrieb what's the other breaking change? #142352 has been merged for over a month..

[RalfJung]

@Noratrieb what's the other breaking change? #142352 has been merged for over a month..

Uh, but then why did Miri CI only start failing today?

Something changed in 9748d87...b56aaec.

[RalfJung]

The breaking change was probably #144218.

[WaffleLapkin]

The only relevant change I see is #144218, but that doesn't explain anything, it should have been failing with #142352, I don't see how it could have accepted c-int-width being a string...

[RalfJung]

It might be that the field of wrong type just got ignored, which is exactly why Nora ported this to serde.

[WaffleLapkin]

You are right indeed. The code in #142352:

                if let Some(s) = obj.remove(name).and_then(|b| b.as_u64()) {
                    base.$key_name = s as $int_ty;
                }

and_then(|b| b.as_u64()) makes it so non-integers are ignored.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[WaffleLapkin]

#145909 got merged, so this should be good to go. I can't imagine what else can break ^^'

[ojeda]

@WaffleLapkin It seems to be on S-waiting-on-author

[WaffleLapkin]

@bors r=Noratrieb

[bors]

📌 Commit 2d8cb59 has been approved by Noratrieb

It is now in the queue for this repository.

[WaffleLapkin]

yay :3

[koute]

Just wanted to say, I'm really disappointed that such a trivial breaking charge was merged without any deprecation period and without making it just accept both forms for compatibility (which also would have been trivial), which would save a lot of pain for a lot of people. Not everyone wants to live on the bleeding edge of MSRV, and some people care that their software works on a wide range of rustc versions.

[RalfJung]

Hm yeah maybe we should have accepted both forms for a while. OTOH, without a lint hardly anyone would have noticed so there would still have been breakage later when the old form is removed, and a lint would have been more extra work. We have always been very clear that there is no stability on nightly.

[koute]

Be that as it may, this is just pretty much an aesthetic change with no real practical benefit besides "it's nicer this way", it was "stable" and worked for years (even if not officially stabilized), there's no non-nightly alternative so those who need it are forced to use it, and many people depend on it. Doesn't the cost-benefit ratio here seem very skewed to you here? Sure, it would be extra work to make it temporarily accept both forms and add a lint for a few releases, but that extra work wasn't eliminated, just multiplied and pushed somewhere else, because now everyone who uses this and doesn't want to require a bleeding edge MSRV will now have to do the extra work to autodetect the compiler and support both.

Anyway, I apologize if I may seem blunt; I know you're doing a thankless job. I just wish this would have been handled better; I suppose it's too late now to do anything about it?

[RalfJung]

I suppose it's too late now to do anything about it?

Someone could make a PR to accept both forms, if you still think that's useful. I think that'd be the first time we do a compatibility transition for the target JSON format so the compiler team would have to discuss whether they consider that precedent a good idea.

[Darksonn]

The target.json format changes pretty often. Three changes come to mind for me:

• The data-layout string sometimes needs to be updated.
• We needed to specify rustc-abi for x86 targets recently.
• And this change with the target-pointer-width field.

So anyone using target.json with multiple compiler versions will have to use some sort of conditional logic when generating it, or just have multiple copies of it, and that's just how it is.

If this particular case is the first time it happened for you, then I understand that it seems frivolous, and if it otherwise never changed in practice I might agree. But it changes often enough that I don't think this kind of thing makes sense.

[RalfJung]

Another recent change is that c_int_width was changed from a string to an integer -- that's in fact what motivated this change. (However, apparently JSON targets in the wild don't often set c_int_width so this didn't cause much fallout.)