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

Add gnullvm targets to test.yml #2974

Closed
wants to merge 2 commits into from
Closed

Add gnullvm targets to test.yml #2974

wants to merge 2 commits into from

Conversation

jeremyd2019
Copy link
Contributor

@jeremyd2019 jeremyd2019 commented Apr 5, 2024

There are no prebuilt std for these targets yet, so have to use nightly, install rust-src, and use build-std.

Known failures:

  • tool_yml is out of date. I wanted to concentrate on the workflow itself, and then can update the generator there.
  • sample_component_json_validator and sample_component_json_validator_winrt fail on i686-pc-windows-gnullvm, because they wind up pulling in windows-targets 0.48 and that version is known not to support i686-pc-windows-gnullvm. I don't know how to work around this.
  • sample_component_json_validator_winrt_client and test_component_client fail on both i686 and x86_64-pc-windows-gnullvm, due to "class not registered" error. This seems to be a side-effect of explicitly specifying a target to cargo.

@jeremyd2019
Copy link
Contributor Author

One thing I know that is different with the llvm-mingw toolchain (and CLANG* toolchain in msys2, which is pretty similar) is that it does not embed a default manifest. The gcc toolchains do (https://sourceware.org/git/cygwin-apps/windows-default-manifest.git), and I'm sure msvc does. This seems like something winrt might require.

@kennykerr
Copy link
Collaborator

because they wind up pulling in windows-targets 0.48

All repo crates should depend on the current/latest version in the repo.

for unknown reasons (get class not registered error)

They probably need a Visual Studio dev command prompt, but only for running MIDLRT (for now).

and I'm sure msvc does

MSVC does not embed a manifest by default, nor is this needed for COM or WinRT.

@jeremyd2019
Copy link
Contributor Author

because they wind up pulling in windows-targets 0.48

All repo crates should depend on the current/latest version in the repo.

It appears to me to be (at least):
sample_component_json_validator -> jsonschema -> parking_lot -> parking_lot_core ->windows-targets 0.48.5

@jeremyd2019
Copy link
Contributor Author

for unknown reasons (get class not registered error)

They probably need a Visual Studio dev command prompt, but only for running MIDLRT (for now).

Locally (I'm manually running the tests on aarch64-pc-windows-gnullvm), I get a panic due to not finding MIDLRT. This is different than on the github runner, where MSVC is present, and it gives

running 1 test
test test ... FAILED

failures:

---- test stdout ----
Error: Error { code: HRESULT(0x80040154), message: "Class not registered" }


failures:
    test

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `-p test_component_client --lib`

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 5, 2024

on aarch64-pc-windows-gnullvm, the only failure that didn't seem to be due to not having msvc installed was test_arch. That is probably expected on arm64, it was using KNONVOLATILE_CONTEXT_POINTERS that was configured out (it suggested a similar name KNONVOLATILE_CONTEXT_POINTERS_ARM64). Of course, not having msvc caused the component tests to fail, so I didn't get to find out if the 'class not registered' error happens there or not.

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 5, 2024

for unknown reasons (get class not registered error)

I tested this with a rust that is hosted on x86_64-pc-windows-gnullvm, and it works. It fails if I add --target x86_64-pc-windows-gnullvm. So it is probably some sort of artifact of when it thinks its cross-compiling. Notably, there is no test_component.dll in target/x86_64-pc-windows-gnullvm/debug/deps, while there is in target/debug/deps. I've tried in CI explicitly specifying --target ${{ matrix.target }} to the test_component_client test, and this fails for all targets with class not registered.

@kennykerr
Copy link
Collaborator

kennykerr commented Apr 6, 2024

Finding the DLL is not easy because of this: rust-lang/cargo#9661 - that accounts for the "class not registered" errors.

@jeremyd2019
Copy link
Contributor Author

OK, so now I know why the tests fail. How would I make sure the tests that are expected to pass are passing, without messing things up too much (clearly, just "commenting" out some tests by turning them into strings in the yml file is not the answer)

@kennykerr
Copy link
Collaborator

All the tests are expected to pass. 🙂

@jeremyd2019
Copy link
Contributor Author

Let me rephrase then. What do I need to do to get this PR into an acceptable state, so that as much as possible is tested on gnullvm? Keep in mind I am not a rust developer, I basically just poke at it when it breaks in msys2's packaging of it or other packages that use it.

@jeremyd2019
Copy link
Contributor Author

/cc @mati865 who is moving -gnullvm forward in rust itself.

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 7, 2024

It occurred to me last night that the dll ends up in target/debug/deps while normally things end up in target/${{ matrix.target }}/debug/deps if --target is specified. Perhaps because the dependency in component_client/Cargo.toml is listed as a build requirement, rust is deciding to compile it for the "host" rather than the "target".

There are no prebuilt std for these targets yet, so have to use nightly,
install rust-src, and use build-std.
As a build dependency, it is being built for the "host", which then causes
it to be placed in a different directory than the component_client test
if an explicit target is specified to cargo.
@jeremyd2019
Copy link
Contributor Author

Changing that to a "normal" dependency vs a build-dependency solves that "class not registered" error. So that just leaves the i686 error due to windows-targets 0.48 being pulled in (indirectly) by the dependency on jsonschema in the sample_component_json_validator* tests.

I don't know if there is a way to force cargo to ignore the version specification and always just use the version of the crate in this repository. In my experience, cargo really wants to enforce semver and any method I've thought of to try to mess with that has ended in failure.

@mati865
Copy link
Contributor

mati865 commented Apr 7, 2024

It occurred to me last night that the dll ends up in target/debug/deps while normally things end up in target/${{ matrix.target }}/debug/deps if --target is specified. Perhaps because the dependency in component_client/Cargo.toml is listed as a build requirement, rust is deciding to compile it for the "host" rather than the "target".

When cross compiling (or using -Zbuild-std) Cargo will use target/<triple>/<build_type> instead of target/<build_type> which is reserved for host toolchain.

Changing that to a "normal" dependency vs a build-dependency solves that "class not registered" error

Build dependencies are built only for the host so they are placed in target/<build_type>, dev and normal dependencies are built for the target so they are put to target/<triple>/<build_type> (dev dependencies are used only when building tests).

I don't know if there is a way to force cargo to ignore the version specification and always just use the version of the crate in this repository. In my experience, cargo really wants to enforce semver and any method I've thought of to try to mess with that has ended in failure.

No, Cargo is very strict about it. Technically it might be possible using deprecated replace but that could be fragile.

@kennykerr
Copy link
Collaborator

parking_lot_core has already moved to 0.52 but needs a release to update crates.io...

@jeremyd2019
Copy link
Contributor Author

parking_lot_core has already moved to 0.52 but needs a release to update crates.io...

☹️ it looks like the relaxed build.rs checks that sort of accidentally allowed i686-pc-windows-gnullvm to use the windows_i686_gnu crate wasn't present until 0.53, so 0.52 would still fail the same way.

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 7, 2024

No, Cargo is very strict about it. Technically it might be possible using deprecated replace but that could be fragile.

I looked at the docs for that, but they say:

Note that when a crate is overridden the copy it’s overridden with must have both the same name and version

I figured it wouldn't hurt to try it anyway, but as expected:

error: failed to get `windows-targets` as a dependency of package `windows-sys v0.48.0`
    ... which satisfies dependency `windows-sys = "^0.48"` of package `mio v0.8.11`
    ... which satisfies dependency `mio = "^0.8.6"` of package `test_lib v0.0.0 (D:\a\windows-rs\windows-rs\crates\tests\lib)`

Caused by:
  no matching package for override `https://github.com/rust-lang/crates.io-index#windows-targets@0.48.5` found
  location searched: D:\a\windows-rs\windows-rs\crates\libs\targets
  version required: =0.48.5

@@ -10,6 +10,7 @@ on:

Copy link
Collaborator

Choose a reason for hiding this comment

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

All the stuff here to fix the environment for gnullvm should probably be in the fix-environment action...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was basing this on what was already done in cross.yml. The only "environment" thing this does that that does not is adding some extra to ~/.cargo/config. Would you like that moved to fix-environment, the whole setup of the llvm toolchain moved to fix-environment, or possibly the setup of the llvm toolchain in a new action (it seems like it should be possible to reuse that between here and cross.yml)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Chatting with @riverar the non-MSVC testing is a bit of muddle and needs to get cleaned up. Will get back to you on this.

@kennykerr
Copy link
Collaborator

@riverar will take a look at getting this working in the fix-environment action. Closing this for now.

@kennykerr kennykerr closed this Apr 17, 2024
@jeremyd2019
Copy link
Contributor Author

I am hoping after rust-lang/rust#121712 it will become possible to just rustup target add the gnullvm targets rather than having to use -Z build-std

@jeremyd2019 jeremyd2019 deleted the add-gnullvm-to-tests branch April 17, 2024 17:15
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