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

Print URL on failure to upload. #8479

Closed
wants to merge 1 commit into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jul 12, 2020

When setting up a local copy of crates.io, I got error output like this:

Uploading uploadtest v0.1.0 (/home/jsha/learnrust/uploadtest)
error: [7] Couldn't connect to server

I was pretty sure cargo was getting the wrong URL for my local registry,
so I wanted to amend the error output to include the failed URL. Now the
output looks like this:

Uploading uploadtest v0.1.0 (/home/jsha/learnrust/uploadtest)
error: publishing to http://localhost:8888//api/v1/crates/new: [7] Couldn't connect to server

When setting up a local copy of crates.io, I got error output like this:

   Uploading uploadtest v0.1.0 (/home/jsha/learnrust/uploadtest)
error: [7] Couldn't connect to server

I was pretty sure cargo was getting the wrong URL for my local registry,
so I wanted to amend the error output to include the failed URL. Now the
output looks like this:

   Uploading uploadtest v0.1.0 (/home/jsha/learnrust/uploadtest)
error: publishing to http://localhost:8888//api/v1/crates/new: [7] Couldn't connect to server
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (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 Jul 12, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented Jul 12, 2020

This seems reasonable. I wonder why we don't have an existing test. @alexcrichton or @ehuss is there some reason this should not have a test or any advice on adding one?

@ehuss
Copy link
Contributor

ehuss commented Jul 13, 2020

It should be pretty easy to make a test that has a dummy server that drops the connection. Let me know if you need help or guidance on how to set that up.

I would also suggest using chain_err() instead of map_err, otherwise any error chains might be lost.

@jsha
Copy link
Contributor Author

jsha commented Jul 14, 2020

Thanks for the feedback on chain_err. I'll give that a try. My original version of this did use chain_err(), but I was getting some errors about not having that method. I'll see if I need to tweak some types.

As for testing, yes I'd definitely love some guidance on setting that up. Not sure where to start. Thanks!

@ehuss
Copy link
Contributor

ehuss commented Jul 14, 2020

Ah, I forgot this is crates_io, the method is actually called with_context and you need to import anyhow::Context. Cargo itself has a custom trait for chain_err which we should probably remove someday.

The CONTRIBUTING document has some information on running tests. Cargo uses a library called cargo_test_support which provides a bunch of helpers for writing tests, there is some documentation on using it in the lib.

I would stick the test in publish.rs. There are a bunch of tests in there for you to get a feel for what they look like (though none of them do networking).

I would roughly structure it as:

  1. Set up a tcp server to listen for a request. Spawn a thread that will accept a connection and drop it.
    There are several tests that use TcpListener::bind, so check those out for examples.
  2. Set up a custom registry with the API URL pointing to the server address from step 1.
    There are functions in registry.rs for setting up registries (init_registry would be a good start).
  3. Set up a custom .cargo/config with a [registries] entry pointing to your custom index (use a file url from generate_url()).
  4. Create a demo package to publish.
    See publish::simple for a basic example of creating a project and publishing it.
  5. Try to cargo publish --registry myregistry and check the error output.
    The with_stderr should capture the new error message, and you'll need to add .with_status(101) to tell it you are expecting it to fail.

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2020
@ehuss
Copy link
Contributor

ehuss commented Aug 31, 2020

Ping @jsha. Just checking if you have any questions on writing the test.

@jsha
Copy link
Contributor Author

jsha commented Sep 20, 2020

Thanks for your patience! I've just now come back to this, and I'm running into trouble getting the tests to run, even on the master branch. Here's some environment info:

$ export CFG_DISABLE_CROSS_TESTS=1
$ $ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/jsha/.rustup

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu
nightly-x86_64-unknown-linux-gnu (default)

active toolchain
----------------

nightly-x86_64-unknown-linux-gnu (default)
rustc 1.46.0-nightly (a8cf39911 2020-06-21)
$ rustc -vV
rustc 1.46.0-nightly (a8cf39911 2020-06-21)
binary: rustc
commit-hash: a8cf3991177f30694200002cd9479ffbbe6d9a1a
commit-date: 2020-06-21
host: x86_64-unknown-linux-gnu
release: 1.46.0-nightly
LLVM version: 10.0

$ cargo test transitive_new_major
    Finished test [unoptimized + debuginfo] target(s) in 0.26s
     Running target/debug/deps/cargo-19930bc3627ca11d

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 27 filtered out

     Running target/debug/deps/cargo-a7e00134ee4010a3

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/build_std-d7a1357d664871b5

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out

     Running target/debug/deps/internal-c2c2c05ff2b04acf

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out

     Running target/debug/deps/testsuite-9948a6b38b78f501

running 1 test
test patch::transitive_new_major ... FAILED

failures:

---- patch::transitive_new_major stdout ----
running `/home/jsha/rust/cargo/target/debug/cargo build`
thread 'patch::transitive_new_major' panicked at '
Expected: execs
    but: exited with exit code: 101
--- stdout

--- stderr
error: process didn't exit successfully: `rustc -vV` (exit code: 1)
--- stderr
error: no default toolchain configured

', crates/cargo-test-support/src/lib.rs:832:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    patch::transitive_new_major

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1954 filtered out

error: test failed, to rerun pass '--test testsuite'

I believe I've followed the instructions at https://github.com/rust-lang/cargo/blob/master/CONTRIBUTING.md#running-tests, though it's possible I've made a mistake. When I run cargo test, many of the tests fail; if I run them one by one, they generally give the above error.

@ehuss
Copy link
Contributor

ehuss commented Sep 22, 2020

Hm, that's strange. I've only seen that if you execute cargo directly (like target/debug/cargo test), which circumvents and confuses rustup. Maybe check which cargo and which rustc to make sure they are pointing to the binaries in $HOME/.cargo/bin (I think $HOME is important, as I think rustup uses that env var to find the toolchains). Also 2020-06-21 looks pretty old, maybe try updating? Also make sure no rustup environment variables are set like RUSTUP_TOOLCHAIN or RUSTUP_HOME.

@bors
Copy link
Contributor

bors commented Dec 7, 2020

☔ The latest upstream changes (presumably #8950) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@ehuss
Copy link
Contributor

ehuss commented Jan 13, 2021

Ping @jsha: just wondering if you were able to figure out what issue you were having with the tests and if you had any questions on writing the new test.

@jsha
Copy link
Contributor Author

jsha commented Jan 13, 2021

Hi @ehuss! Thanks for the ping. I'm afraid I have not had time to circle back and figure out what the issue with the tests was. I'm going to close this out for now but may try again later.

@jsha jsha closed this Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants