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

upgrade Rust to 1.43.1 #9681

Merged
merged 9 commits into from
May 28, 2020
Merged

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented May 2, 2020

Problem

Staying current with Rust stable releases.

Solution

Upgrade to the latest stable release 1.43.0.

Result

Native engine builds and passes tests just fine.

./pants test src/python:: fails eventually with:

ERROR: Double requirement given: pex>=2.1.9 (already in pex==2.1.9, name='pex')

I have the full log, going to see if CI sees the same result before pursuing.

@tdyas tdyas marked this pull request as ready for review May 3, 2020 01:47
@tdyas
Copy link
Contributor Author

tdyas commented May 3, 2020

With clippy fixes, everything passes except the self-test which appears to be failing because of failing to download some artifacts.

@Eric-Arellano
Copy link
Contributor

except the self-test which appears to be failing because of failing to download some artifacts.

Restarted to see if this was network flakiness.

@tdyas
Copy link
Contributor Author

tdyas commented May 4, 2020

Failed for infra reasons again:

$ ./build-support/bin/get_ci_bootstrapped_pants_pex.sh ${AWS_BUCKET} ${BOOTSTRAPPED_PEX_KEY_PREFIX}.${BOOTSTRAPPED_PEX_KEY_SUFFIX}

fatal error: An error occurred (403) when calling the HeadObject operation: Forbidden

The command "./build-support/bin/get_ci_bootstrapped_pants_pex.sh ${AWS_BUCKET} ${BOOTSTRAPPED_PEX_KEY_PREFIX}.${BOOTSTRAPPED_PEX_KEY_SUFFIX}" failed and exited with 1 during .

@Eric-Arellano
Copy link
Contributor

Failed for infra reasons again:

This happens when your uploaded pants.pex is stale. You'll need to pull master to trigger a new CI run.

But, the restart I did didn't help. It had the same error as before.

@tdyas
Copy link
Contributor Author

tdyas commented May 4, 2020

You'll need to pull master to trigger a new CI run.

There was a merge conflict any way, so I just rebased and force pushed off of master.

@stuhood
Copy link
Sponsor Member

stuhood commented May 4, 2020

Failing to fetch grpc is weird. I'll wipe the cache for this.

EDIT: Wiped and restarted those shards.

@tdyas
Copy link
Contributor Author

tdyas commented May 6, 2020

It is still failing because of the gRPC issue (assuming the cache wipe occurred).

    Updating git repository `https://github.com/pantsbuild/grpc-rs.git`

error: failed to get `grpcio` as a dependency of package `bazel_protos v0.0.1 (/home/travis/build/pantsbuild/pants/src/rust/engine/process_execution/bazel_protos)`

@tdyas
Copy link
Contributor Author

tdyas commented May 13, 2020

Still an infra failure at https://travis-ci.com/github/pantsbuild/pants/jobs/333012689#L775 related to grpcio and trying to clone the repository.

@stuhood
Copy link
Sponsor Member

stuhood commented May 13, 2020

Still an infra failure at https://travis-ci.com/github/pantsbuild/pants/jobs/333012689#L775 related to grpcio and trying to clone the repository.

The error I see in there is:

error reading from the zlib stream; class=Zlib (5)

I'd look at the differences between the OS packages installed on the lint shard and on the bootstrap shards.

@tdyas
Copy link
Contributor Author

tdyas commented May 16, 2020

From what I can tell, from cargo 1.42.0 to 1.43.0, the version of the libgit2-sys crate went from 0.10.0 to 0.12.0. A similar error message re zlib was reported to that crate in rust-lang/git2-rs#546, but resolved as "spurious."

@tdyas
Copy link
Contributor Author

tdyas commented May 16, 2020

1.43.1 came out, appears to fix it locally on my Ubuntu VM. Will see if CI agrees.

@stuhood
Copy link
Sponsor Member

stuhood commented May 28, 2020

I repro this failure locally if I remove ~/.cache/pants/rust (warning: all the things will be recompiled if you do this!)

EDIT: Also, it looks like it's been reported here: rust-lang/cargo#8258

EDIT2: I'll try working around this by filtering out that submodule.

@stuhood stuhood changed the title upgrade Rust to 1.43.0 upgrade Rust to 1.43.1 May 28, 2020
@tdyas
Copy link
Contributor Author

tdyas commented May 28, 2020

EDIT: Also, it looks like it's been reported here: rust-lang/cargo#8258

Ah good, somebody did a more in-depth look at the issue. As reported earlier, I had only ended up at the libgit2-sys version bump when I last looked at this, but gave up when nothing obvious stood out.

I wonder if we could just fork that problematic repo to somewhere not on the chromium.googlesource.com server, and point Cargo there instead.

@tdyas
Copy link
Contributor Author

tdyas commented May 28, 2020

And a libgit2 issue was opened as well: libgit2/libgit2#5525

Seems like they isolated it to one commit. Of course, that commit is large change to their HTTP code.

# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
@stuhood stuhood requested review from stuhood and jsirois May 28, 2020 04:41
@stuhood
Copy link
Sponsor Member

stuhood commented May 28, 2020

It looks like the submodule changes might work? Rebasing and sanity checking and will push shortly.

Tom Dyas added 7 commits May 27, 2020 21:42
fixed by upgrading derivative to latest

Clippy output:

    Checking process_execution v0.0.1 (/Users/tdyas/TC/pants/src/rust/engine/process_execution)
error: this match could be written as a `let` statement
  --> process_execution/src/remote.rs:37:1
   |
37 | #[derivative(Debug)]
   | ^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> process_execution/src/lib.rs:7:3
   |
7  |   clippy::all,
   |   ^^^^^^^^^^^
   = note: `#[deny(clippy::match_single_binding)]` implied by `#[deny(clippy::all)]`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
help: consider using `let` statement
   |
37 | let Derivative = #[derivative(Debug)];
38 | Derivative

See mcarton/rust-derivative#58. Fix was released in vw2.1.1.
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
…ust-lang/cargo#8258.

# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@stuhood stuhood merged commit bf470d4 into pantsbuild:master May 28, 2020
@tdyas tdyas deleted the upgrade_rust_to_1.43.0 branch May 28, 2020 06:35
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.

None yet

3 participants