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

Fix Travis Windows build #3418

Merged
merged 9 commits into from Jul 15, 2019
Merged

Conversation

phansch
Copy link
Member

@phansch phansch commented Nov 7, 2018

Closes #3306

@mati865
Copy link
Contributor

mati865 commented Nov 7, 2018

If that doesn't having binary representation of .rs, .stderr and test output could shed some light.

@mati865
Copy link
Contributor

mati865 commented Nov 7, 2018

Strange, cat after dos2unix shows the line endings haven't changed (still are CRLF). dos2unix stdout is totally out of place and shows only a few lines.

@phansch
Copy link
Member Author

phansch commented Nov 7, 2018

I don't know what's going on with the cat 🐈 and dos2unix =(

Looking at the test output again, specifically the diffs: https://travis-ci.org/rust-lang-nursery/rust-clippy/jobs/451994929#L1102
They are indented by one space. This seems to come from compiletest-rs/src/runtest.rs#L2502, the Both case.

I guess I will try it with a patched compiletest-rs to print newline characters using {:?} instead of {}.

@phansch
Copy link
Member Author

phansch commented Nov 7, 2018

oh wait 🤦‍♂️! Looking at the past logs again, it looks like dos2unix only formats the first directory that's globbed from **/*?

dos2unix: converting file tests/ui/author/for_loop.stderr to Unix format...
dos2unix: converting file tests/ui/author/matches.stderr to Unix format...
dos2unix: converting file tests/ui/author/call.rs to Unix format...
dos2unix: converting file tests/ui/author/for_loop.rs to Unix format...
dos2unix: converting file tests/ui/author/matches.rs to Unix format...

I'm going to give it a try with some sort of find/xargs loop

@phansch phansch changed the title Fix Travis Windows build WIP: Fix Travis Windows build Nov 7, 2018
@phansch
Copy link
Member Author

phansch commented Nov 7, 2018

Marked as WIP because this will need a rebase tomorrow and Travis doesn't start right now either. I'm pretty convinced that the updated find command will make the Windows UI tests pass.

@mati865
Copy link
Contributor

mati865 commented Nov 7, 2018

Well done, looks better:

failures:
    [ui] ui\author.rs
    [ui] ui\author\call.rs
    [ui] ui\author\for_loop.rs
    [ui] ui\trailing_zeros.rs
test result: FAILED. 219 passed; 4 failed; 1 ignored; 0 measured; 0 filtered out

@phansch
Copy link
Member Author

phansch commented Nov 8, 2018

The UI and UI-Toml tests pass now. The dogfood tests are failing now:

++++ pwd
+++ CLIPPY='/c/Users/travis/build/rust-lang-nursery/rust-clippy/target/debug/cargo-clippy clippy'
+++ /c/Users/travis/build/rust-lang-nursery/rust-clippy/target/debug/cargo-clippy clippy --all-targets --all-features -- -D clippy::all -D clippy::internal -Dclippy::pedantic
error: failed to run `rustc` to learn about target-specific information
Caused by:
  process didn't exit successfully: `C:\Users\travis\build\rust-lang-nursery\rust-clippy\target\debug\clippy-driver.exe rustc - --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (exit code: 3221225781)

I wonder what's up with --crate-name ___?

@mati865
Copy link
Contributor

mati865 commented Nov 8, 2018

Try adding toolchain bin dir to the PATH: #1422 (comment)
For Linux it's done with export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib but Windows wants libs in the PATH and doesn't use LD_LIBRARY_PATH.

Other people also hit this issue: https://github.com/rust-lang-nursery/rust-clippy/search?q=3221225781&type=Issues

@flip1995
Copy link
Member

flip1995 commented Nov 8, 2018

In the appveyor config we add the binaries with https://github.com/rust-lang-nursery/rust-clippy/blob/3bb88775de5a6f8f5c5743dd11af6c820122ccc7/appveyor.yml#L27 to the PATH. The same should work with the libs.

@mati865
Copy link
Contributor

mati865 commented Nov 8, 2018

Lib won't work for Windows, all dynamic libs are placed in /bin.

@phansch
Copy link
Member Author

phansch commented Nov 9, 2018

That worked! I now also removed the windows build from allow_failures. Thanks for your help =)

@phansch phansch changed the title WIP: Fix Travis Windows build Fix Travis Windows build Nov 9, 2018
ci/base-tests.sh Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

flip1995 commented Nov 10, 2018

Nice, windows passes now! Let's bring this on it's way, but still keep appveyor until travis windows is (more) stable (windows build time not linux build time times 2).

bors r+

bors bot added a commit that referenced this pull request Nov 10, 2018
3418: Fix Travis Windows build r=flip1995 a=phansch

Closes #3306

Co-authored-by: Philipp Hansch <dev@phansch.net>
@bors
Copy link
Contributor

bors bot commented Nov 10, 2018

Build failed

@phansch
Copy link
Member Author

phansch commented Nov 10, 2018

huh, the windows build failed without error. I'm going to restart it.

for reference, because the log will be lost with the restart:

$ if [ -z ${INTEGRATION} ]; then
    if [ "$TRAVIS_OS_NAME" == "linux" ]; then
      . $HOME/.nvm/nvm.sh
      nvm install stable
      nvm use stable
      npm install remark-cli remark-lint
    fi
    if [ "$TRAVIS_OS_NAME" == "windows" ]; then
      choco install windows-sdk-10.0
      find tests/ui/* \( -name '*.rs' -or -name '*.stderr' -or -name '*.stdout' \) -exec dos2unix '{}' +
      find tests/ui-toml/* \( -name '*.rs' -or -name '*.stderr' -or -name '*.stdout' -or -name '*.toml' \) -exec dos2unix '{}' +
    fi
  fi
  

@phansch

This comment has been minimized.

bors bot added a commit that referenced this pull request Nov 10, 2018
3418: Fix Travis Windows build r=flip1995 a=phansch

Closes #3306

Co-authored-by: Philipp Hansch <dev@phansch.net>
@bors

This comment has been minimized.

@phansch
Copy link
Member Author

phansch commented Nov 10, 2018

I guess the allow_failure wouldn't have worked either with that typo in there 🤦‍♂️

@phansch

This comment has been minimized.

@phansch

This comment has been minimized.

@mati865

This comment has been minimized.

@bors

This comment has been minimized.

@flip1995
Copy link
Member

Removing CARGO_INCREMENTAL=0 slowed down the build by another 8 minutes... Let's run a try run to see whether it works with bors and whether the build time was a coincidence.

@bors try

@bors
Copy link
Collaborator

bors commented Jul 13, 2019

⌛ Trying commit 876a7e1 with merge ebfc081...

bors added a commit that referenced this pull request Jul 13, 2019
@bors
Copy link
Collaborator

bors commented Jul 13, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: ebfc081

@phansch
Copy link
Member Author

phansch commented Jul 14, 2019

We could maybe skip the dogfood tests on windows? I'm not sure but I guess that's where most of the time is spent in the windows build.

This reverts commit 876a7e1.

Using incremental build on windows increases the build time on travis by
about 8 minutes.
@flip1995
Copy link
Member

flip1995 commented Jul 14, 2019

Build time is up by 8 minutes without CARGO_INCREMENTAL=0. So let's leave this in.

Removing dogfood is a good idea, since the results should be the same on windows and linux/macos.


To improve the build time of every build: Maybe we could also cache RTIM, to not have to install it every time?

@flip1995
Copy link
Member

Without dogfood we now have about the same build times across the platforms.

@bors try


I think the important question before merging this is:
Should we still allow failures on windows builds or should we yolo it? 😄

@bors
Copy link
Collaborator

bors commented Jul 14, 2019

⌛ Trying commit ce2a7b0 with merge 25b51f8...

bors added a commit that referenced this pull request Jul 14, 2019
@bors
Copy link
Collaborator

bors commented Jul 14, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: 25b51f8

@flip1995
Copy link
Member

@bors try

@bors
Copy link
Collaborator

bors commented Jul 14, 2019

⌛ Trying commit c61e4e1 with merge 2655637...

bors added a commit that referenced this pull request Jul 14, 2019
@flip1995
Copy link
Member

Caching RTIM only saves 2-5 minutes of build time, but stores caches of the size of ~40MB. I don't think this is worth it.

@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 14, 2019
@flip1995
Copy link
Member

This should be ready for merge now. Last question:

Should we

  1. allow failures on the windows build 👀
  2. or yolo it? 🚀

@phansch
Copy link
Member Author

phansch commented Jul 15, 2019

If it works, it works 🤷‍♂️ Thanks for pushing this through!

@bors r=me,flip1995

@bors
Copy link
Collaborator

bors commented Jul 15, 2019

📌 Commit ce2a7b0 has been approved by me,flip1995

@bors
Copy link
Collaborator

bors commented Jul 15, 2019

⌛ Testing commit ce2a7b0 with merge 48b50e8...

bors added a commit that referenced this pull request Jul 15, 2019
@bors
Copy link
Collaborator

bors commented Jul 15, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: me,flip1995
Pushing 48b50e8 to master...

@bors bors merged commit ce2a7b0 into rust-lang:master Jul 15, 2019
@phansch phansch deleted the add_travis_windows_build branch July 15, 2019 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Travis Windows: Secret ENV vars break the build
4 participants