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

Need regression test for wasm linker override=clang (108910) #109728

Closed
pnkfelix opened this issue Mar 29, 2023 · 14 comments
Closed

Need regression test for wasm linker override=clang (108910) #109728

pnkfelix opened this issue Mar 29, 2023 · 14 comments
Assignees
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Mar 29, 2023

As noted in #108996 (comment), I don't want to delay the fix further on the CI issues I was having with my regression test.

This issue is just tracking a (hopefully simple?) work item to make such a regression test, e.g. using the one from 568b722 after figuring out how to make the CI happy (see error CI posted here: #108996 (comment) )

@pnkfelix pnkfelix added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 29, 2023
@agaraman0
Copy link

@pnkfelix i would like to pick this

@jyn514
Copy link
Member

jyn514 commented Apr 3, 2023

@agaraman0 go for it :) you can use @rustbot claim to assign yourself. See https://rustc-dev-guide.rust-lang.org/getting-started.html for more instructions.

@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 5, 2023

Full disclaimer, I'm not an experienced contributor to open source, nor will I pretend to be, so take the below with a grain of salt:

If we're just checking to see if the link-arg=-nostartfiles is being emitted, would it suffice to just capture the link flags (e.g. using RUSTC_LOG=rustc_codegen_ssa::back::link=info) and check if this option is present through a simple string find? That way, even if clang isn't present in the CI environment, the functionality can still be verified. This is certainly brittle to some degree, so not ideal...

Although it does seem strange that clang isn't available... Your wording of "make the CI happy" seems to imply that there's a workaround of sorts - if so, could you provide a few pointers? I'd be happy to investigate

@jyn514
Copy link
Member

jyn514 commented Apr 5, 2023

@ndrewxie feel free to install clang on any of the CI builders, look for apt install in src/ci for examples.

@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 5, 2023

@jyn514 Oh dang I didn't expect to have the perms to do that in a CI environment. Thank you! I'll test that approach and add a test :)

@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 6, 2023

@ndrewxie feel free to install clang on any of the CI builders, look for apt install in src/ci for examples.

Hmm, there appears to already be a script that installs clang, located in /src/ci/scripts/install-clang.sh. However, it chooses to not install it for linux under the assumption that every system would already have clang installed and configured properly.

Note that we don't install clang on Linux since its compiler story is just so different. Each container has its own toolchain configured appropriately already.

Would it be safe for me to apt install it anyways, or is there a risk that clang acquired from the package manager would be configured improperly and mess something up?

@jyn514
Copy link
Member

jyn514 commented Apr 6, 2023

As long as the new test passes and no other test regresses, it's probably ok. We don't have a hard dependency on the version of clang like we do for LLVM and the bootstrap compiler.

@b-ncMN
Copy link
Contributor

b-ncMN commented Apr 18, 2023

Is this being worked on ? I'd like to give it a try

@ndrewxie
Copy link
Contributor

Is this being worked on ? I'd like to give it a try

Afaik it's not being worked on. Good luck!

@b-ncMN
Copy link
Contributor

b-ncMN commented Apr 26, 2023

@rustbot claim

@b-ncMN
Copy link
Contributor

b-ncMN commented May 1, 2023

I'm not sure about the goal, after checking some stuff out
The goal would be to take the test (this test 568b722), and make it work by installing clang?

@jyn514
Copy link
Member

jyn514 commented May 1, 2023

@InfRandomness yup, that sounds right :)

@reez12g
Copy link
Contributor

reez12g commented Sep 29, 2023

@rustbot claim

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 2, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 8, 2023
…acrum

add test for wasm linker override=clang

addressing rust-lang#109728
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 9, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 9, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 14, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 17, 2023
@Dylan-DPC
Copy link
Member

Closing as #116264 resolves it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants