Skip to content

Expose a way to set cargo:rustc-link-arg? #69

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

Closed
DavisVaughan opened this issue Sep 26, 2024 · 7 comments
Closed

Expose a way to set cargo:rustc-link-arg? #69

DavisVaughan opened this issue Sep 26, 2024 · 7 comments

Comments

@DavisVaughan
Copy link

Hi there, great library!

We are currently using this library to embed an application manifest into our main ark.exe executable on windows:
https://github.com/posit-dev/ark/blob/7e957a2930194ca3dde8fb828d4819f37e2bb5e7/crates/ark/build.rs#L38-L43

This works great! It allows us to enable UTF-8 and declare compatibility with modern Windows systems, i.e.:
https://github.com/posit-dev/ark/blob/main/crates/ark/resources/manifest/ark.exe.manifest

What doesn't work so great is unit testing. When we run unit tests, we get an executable for ark in target\debug\deps\ark-<stuff>.exe It seems like that executable does not get embedded with the application manifest! We really need the application manifest in there, so that we can run unit tests that test UTF-8 behavior to ensure we have it working correctly.

After doing much research, IIUC the problem is that cargo:rustc-link-arg-bins is used here:

println!("cargo:rustc-link-arg-bins={}", out_file);

According to the cargo book, that corresponds to "Passes custom flags to a linker for binaries." but apparently the unit test binaries don't count!

I also tried compile_for_tests(), which sets cargo:rustc-link-arg-tests for "Passes custom flags to a linker for tests", which in theory sounds great...but is apparently broken for unit tests?? rust-lang/cargo#10937

Finally I was able to get this to work by dropping embed_resource and "manually" setting cargo:rustc-link-arg for "Passes custom flags to a linker for benchmarks, binaries, cdylib crates, examples, and tests."

fn set_windows_exe_options() {
    static WINDOWS_MANIFEST_FILE: &str = "ark.exe.manifest";

    let mut manifest = std::env::current_dir().unwrap();
    manifest.push("resources");
    manifest.push("manifest");
    manifest.push(WINDOWS_MANIFEST_FILE);

    //println!("cargo:rerun-if-changed={WINDOWS_MANIFEST_FILE}");
    // Embed the Windows application manifest file.
    println!("cargo:rustc-link-arg=/MANIFEST:EMBED");
    println!(
        "cargo:rustc-link-arg=/MANIFESTINPUT:{}",
        manifest.to_str().unwrap()
    );
    // Turn linker warnings into errors.
    println!("cargo:rustc-link-arg=/WX");
}

This does work, and this /MANIFEST:EMBED setup is what rustc does (https://github.com/rust-lang/rust/blob/master/compiler/rustc/build.rs), so I feel comfortable going this route if I have to, but I'd like to still use embed_resource if I can.

Would it be possible to expose cargo:rustc-link-arg as an endpoint too?

@nabijaczleweli
Copy link
Owner

that corresponds to "Passes custom flags to a linker for binaries." but apparently the unit test binaries don't count!

This reads a little weird but I think it makes sense within the cargo ontology of libraries/binaries/tests ([lib]/[[bin]]/[[test]]/#[test]/it broke down for me but you get my point).

Especially since the book lists cargo::rustc-link-arg-tests=FLAG (and -tests) just below.

So -bin/-bins is for [[bin]]aries, -tests for [[test]]s, and #[test]s get nothing?

also doctests fit into this somehow.

the design of this, as you must've noticed when you read the book, tested this, and skimmed rust-lang/cargo#10937, is concise and well-thought-out, so I am loathe to make any change to the generated cargo directives at all, and I already support https://github.com/nabijaczleweli/rust-embed-resource/releases/tag/v1.6.13 and https://github.com/nabijaczleweli/rust-embed-resource/releases/tag/v2.4.3. i certainly hope to avoid having a third branch (if only because finding three bars that work together would be even harder)

it's exceedingly ironic that despite all of those itemised ones, the "just fuckin put it everywhere" endpoint is actually what you need here

i agree with your analysis, that is what you want here, and it shouldn't cause another split

@DavisVaughan
Copy link
Author

"just fuckin put it everywhere" definitely came out of my mouth when I finally tried that after a few hours of guess work and research

@DavisVaughan
Copy link
Author

DavisVaughan commented Sep 26, 2024

I guess what I'd be requesting as a workaround ( / potential feature?) is compile_for_all() which would be exactly like compile_for_tests() internally, except it would target cargo:rustc-link-arg=, where "all" means:

for benchmarks, binaries, cdylib crates, examples, and tests.

Are any of those locations definitely not suitable for the kinds of linker flags this crate is meant to generate?

(the name "all" is a callout to how it is represented in the compiler itself https://github.com/rust-lang/cargo/blob/9d66d13e4479f1d51347c20264713c2d6cd8b4fa/src/cargo/util/context/target.rs#L186-L187)


Is something like compile_for_all() exactly what you are hesitant to add?

@nabijaczleweli
Copy link
Owner

nabijaczleweli commented Sep 26, 2024

as I review the API, compile says this

/// Since rustc 1.50.0, the resource is linked only to the binaries
/// (unless there are none, in which case it's also linked to the library).

        if hasbins && rustc_version::version().expect("couldn't get rustc version") >= rustc_version::Version::new(1, 50, 0) {
            println!("cargo:rustc-link-arg-bins={}", out_file);
        } else {
            // Cargo pre-0.51.0 (rustc pre-1.50.0) compat
            // Only links to the calling crate's library
            println!("cargo:rustc-link-search=native={}", out_dir);
            println!("cargo:rustc-link-lib=dylib={}", prefix);
        }

and prior to e895286 it just did

        comp.compile_resource(&out_dir, &prefix, resource_file.to_str().expect("resource_file not UTF-8"));
        println!("cargo:rustc-link-search=native={}", out_dir);
        println!("cargo:rustc-link-lib=dylib={}", prefix);

(which I think actually put it everywhere? I'm sure the reasoning is recorded in these issues and that it's infohazardous)

so we're truly just going in circles, design-wise

@nabijaczleweli
Copy link
Owner

nabijaczleweli commented Sep 26, 2024

I was less hesitant (extending in this direction is free-rein, my original concern was for changing the output for any of the current functions) and more "trying to find the boundary version that supports this" which is sisyphean every time since rust enthusiasts simply refuse to annotate their tags.

I arrived at compile_for_everything(). Same difference, really. Can you try the current master branch (at least ed238e6)?

@DavisVaughan
Copy link
Author

That is working great for me! Thanks!

@nabijaczleweli
Copy link
Owner

Great, thanks for the detailed report. Released in v2.5.0, no backport applies (API change only).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants