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

build(windows): fix typo in build.rs #3955

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Jul 17, 2024

Cherry-picked from #3898.

The first commit is deliberately failing to demonstrate that the fix is actually working.

See aws/aws-lc-rs#453 (comment) for the whole context:

Another crucial difference between our setups is that we use the following linker option on Windows MSVC as well:

println!("cargo:rustc-link-arg-bin=rustup-init=/WX");

This was originally added by @ChrisDenton to prevent cargo from silencing linkage warnings, which is probably why your CI happens to turn green.

@rami3l rami3l added this to the 1.28.0 milestone Jul 17, 2024
@rami3l rami3l requested a review from djc July 17, 2024 03:48
@djc
Copy link
Contributor

djc commented Jul 17, 2024

It doesn't look like we want to merge this as is? The commits here are a little strange to me.

@rami3l
Copy link
Member Author

rami3l commented Jul 17, 2024

It doesn't look like we want to merge this as is? The commits here are a little strange to me.

@djc There is a typo in the build script that actually invalidated the check that @ChrisDenton wanted to add. If we're linking against aws-lc then this will happen more often...

@ChrisDenton
Copy link
Member

Hm, I don't think that's a typo? It's just that powrprof.dll is no longer being used when switching away from ring? Though maybe I'm misunderstanding the context.

@rami3l
Copy link
Member Author

rami3l commented Jul 17, 2024

Hm, I don't think that's a typo? It's just that powrprof.dll is no longer being used when switching away from ring? Though maybe I'm misunderstanding the context.

@ChrisDenton You accidentally wrote cargo:cargo:rustc-link-arg-bin=rustup-init=/WX with two cargos, so the check didn't trigger.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jul 17, 2024

Ah thanks! I'm confused because when I clicked the "Files Changed" tab I don't see that change. Edit: so I was looking at completely the wrong thing, apparently 😅

@rami3l rami3l force-pushed the fix/windows-build-typo branch from e543987 to d053583 Compare July 17, 2024 14:02
@rami3l rami3l requested a review from djc July 17, 2024 14:04
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

These code changes look good to me, though I'll happily defer to @ChrisDenton on whether these changes are correct (in particular whether we still need delay loading for powrprof).

@rami3l rami3l requested a review from ChrisDenton July 17, 2024 14:05
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

We don't seem to be using powrprof for anything and haven't been for at least a long while (I checked some old rustup-init versions). So I'm good with this.

@rami3l rami3l enabled auto-merge July 17, 2024 14:10
@rami3l rami3l added this pull request to the merge queue Jul 17, 2024
Merged via the queue into rust-lang:master with commit edf5c22 Jul 17, 2024
27 checks passed
@rami3l rami3l deleted the fix/windows-build-typo branch July 17, 2024 14:38
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.

3 participants