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

Rename and fix nolink-with-link-args test #57233

Merged
merged 1 commit into from
Jan 5, 2019
Merged

Rename and fix nolink-with-link-args test #57233

merged 1 commit into from
Jan 5, 2019

Conversation

Smibu
Copy link
Contributor

@Smibu Smibu commented Dec 31, 2018

There are three problems with the nolink-with-link-args test:

  • The test fails when using MSVC. It's caused by the linker-flavor=ld flag which was added in ci: Start running wasm32 tests on Travis #46291.
  • In its comment, this test tests that "link_args are indeed passed when nolink is specified", but the nolink attribute has been removed a long time ago.
  • Pattern has a small typo.

At first I was going to completely remove this test, but there is a closed pull request for that.

So:

  • rename the file as suggested in the closed PR
  • adjust the comment
  • fix typo in the pattern
  • add ignore-msvc.

r? @alexcrichton

There are three problems with the nolink-with-link-args test:

* The test fails when using MSVC. It's caused by the `linker-flavor=ld` flag which was added in #46291.
* In its comment, this test tests that "link_args are indeed passed when nolink is specified", but the `nolink` attribute has been removed [a long time ago](#12826).
* Pattern has a small typo.

At first I was going to completely remove this test, but there is [a closed pull request for that](#21090).

So:

* rename the file as suggested in the closed PR
* adjust the comment
* fix typo in the pattern
* add `ignore-msvc`.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2018
@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Jan 2, 2019

📌 Commit 794b81e has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jan 3, 2019
…=alexcrichton

Rename and fix nolink-with-link-args test

There are three problems with the nolink-with-link-args test:

* The test fails when using MSVC. It's caused by the `linker-flavor=ld` flag which was added in rust-lang#46291.
* In its comment, this test tests that "link_args are indeed passed when nolink is specified", but the `nolink` attribute has been removed [a long time ago](rust-lang#12826).
* Pattern has a small typo.

At first I was going to completely remove this test, but there is [a closed pull request for that](rust-lang#21090).

So:

* rename the file as suggested in the closed PR
* adjust the comment
* fix typo in the pattern
* add `ignore-msvc`.

r? @alexcrichton
kennytm added a commit to kennytm/rust that referenced this pull request Jan 5, 2019
…=alexcrichton

Rename and fix nolink-with-link-args test

There are three problems with the nolink-with-link-args test:

* The test fails when using MSVC. It's caused by the `linker-flavor=ld` flag which was added in rust-lang#46291.
* In its comment, this test tests that "link_args are indeed passed when nolink is specified", but the `nolink` attribute has been removed [a long time ago](rust-lang#12826).
* Pattern has a small typo.

At first I was going to completely remove this test, but there is [a closed pull request for that](rust-lang#21090).

So:

* rename the file as suggested in the closed PR
* adjust the comment
* fix typo in the pattern
* add `ignore-msvc`.

r? @alexcrichton
bors added a commit that referenced this pull request Jan 5, 2019
Rollup of 17 pull requests

Successful merges:

 - #57219 (Remove some unused code)
 - #57229 (Fix #56806 by using `delay_span_bug` in object safety layout sanity checks)
 - #57233 (Rename and fix nolink-with-link-args test)
 - #57238 (Fix backtraces for inlined functions on Windows)
 - #57249 (Fix broken links to second edition TRPL.)
 - #57267 (src/jemalloc is gone, remove its mention from COPYRIGHT)
 - #57273 (Update the stdsimd submodule)
 - #57278 (Add Clippy to config.toml.example)
 - #57295 (Fix 'be be' constructs)
 - #57311 (VaList::copy should not require a mutable ref)
 - #57312 (`const fn` is no longer coming soon (const keyword docs))
 - #57313 (Improve Box<T> -> Pin<Box<T>> conversion)
 - #57314 (Fix repeated word typos)
 - #57326 (Doc rewording, use the same name `writer`)
 - #57338 (rustdoc: force binary filename for compiled doctests)
 - #57342 (librustc_mir: Make qualify_min_const_fn module public)
 - #57343 (Calculate privacy access only via query)

Failed merges:

 - #57340 (Use correct tracking issue for c_variadic)

r? @ghost
@bors bors merged commit 794b81e into rust-lang:master Jan 5, 2019
@Smibu Smibu deleted the rename-and-fix-nolink-test branch January 5, 2019 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants