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

Panic at unwrap if docs were built using CARGO_BUILD_TARGET #38

Closed
ssokolow opened this issue Mar 12, 2019 · 7 comments · Fixed by #102
Closed

Panic at unwrap if docs were built using CARGO_BUILD_TARGET #38

ssokolow opened this issue Mar 12, 2019 · 7 comments · Fixed by #102
Labels
A-cargo Area: Determining directories based on `cargo metadata` C-bug Category: This is a bug C-enhancement Category: This is a new feature S-blocked Status: Blocked on another project

Comments

@ssokolow
Copy link

If CARGO_BUILD_TARGET (or any other method of specifying a non-default target) is set, the docs get placed in target/<target>/doc/<package> rather than target/doc/<package>.

The current cargo installable version of cargo-deadlinks (as of a few minutes ago) panics when it encounters this and you haven't manually specified --dir.

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', src/libcore/result.rs:1009:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:211
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:491
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:398
   6: rust_begin_unwind
             at src/libstd/panicking.rs:325
   7: core::panicking::panic_fmt
             at src/libcore/panicking.rs:95
   8: core::result::unwrap_failed
   9: cargo_deadlinks::main
  10: std::rt::lang_start::{{closure}}
  11: std::panicking::try::do_call
             at src/libstd/rt.rs:59
             at src/libstd/panicking.rs:310
  12: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:102
  13: std::rt::lang_start_internal
             at src/libstd/panicking.rs:289
             at src/libstd/panic.rs:398
             at src/libstd/rt.rs:58
  14: main
  15: __libc_start_main
  16: <unknown>

Instead, it should do two things:

  1. Check for and take CARGO_BUILD_TARGET into account.
  2. Exit gracefully if it can't find the docs.
@ssokolow ssokolow changed the title Panic at unwrap if docs were build using CARGO_BUILD_TARGET Panic at unwrap if docs were built using CARGO_BUILD_TARGET Mar 12, 2019
@hobofan
Copy link
Member

hobofan commented Mar 12, 2019

Not sure if taking CARGO_BUILD_TARGET into account is a viable option. Are there any other environment variable that would tweak the target directory? Also I would assume that most people only set the CARGO_BUILD_TARGET envvar before running cargo doc and not have it exported (so it wouldn't be picked up by cargo-deadlinks anyway). I want to avoid baking to many special cases into it.

But yes, the command should definitely handle an nonexistent directory more gracefully, and probably log out a message that recommends using the --dir argument.

@ssokolow
Copy link
Author

ssokolow commented Mar 12, 2019

Not sure if taking CARGO_BUILD_TARGET into account is a viable option. Are there any other environment variable that would tweak the target directory?

Judging by the docs [1] [2], just three:

  • CARGO_TARGET_DIR for setting the target directory.
  • The two settings from the Cargo config file which can be overridden via environment variables:
    • [build] target becomes CARGO_BUILD_TARGET
    • [build] target-dir becomes CARGO_BUILD_TARGET_DIR

(target sets the target, which changes the default location and target-dir overrides the default location.)

Also I would assume that most people only set the CARGO_BUILD_TARGET envvar before running cargo doc and not have it exported (so it wouldn't be picked up by cargo-deadlinks anyway).

I export it at the top of my justfile to avoid having to include a --target="..." in dozens of different lines and I suspect that sort of thing is how most people use it... as a means to change the default target for a project that is never intended to be built for the host triple.

@ssokolow
Copy link
Author

ssokolow commented Mar 12, 2019

Hmm. It's also possible to configure them by putting cargo config files within the project.

If I'm reading the API docs and the associated source code correctly, the simplest solution would just to be to link against the cargo crate, call Default::default() on cargo::util::config::Config, and then call the target_dir() method.

(That should initialize Cargo's config resolution based on the current working directory and I can see the fallback chain for the aforementioned variables right in the source for target_dir().)

EDIT: ...but I'm not seeing CARGO_BUILD_TARGET in it, so I guess you'd need an extra call to get_path("build.target") if target_dir() returns Ok(None).

(Not that compilcated. Reminds me of using "${XDG_CONFIG_HOME:-$HOME/.config}/appname" to retrieve the proper path for storing application configuration.)

@hobofan
Copy link
Member

hobofan commented Mar 12, 2019

Thanks for the research!

Yeah, you are right, in scripts and CI settings it's probably not that unusual to have the envvar exported. If the ways you describe cover the bulk of the cases, then that looks reasonable to implement.

I'm very much against introducing cargo as a dependency though, as it is huge. Kind of surprised that cargo metadata doesn't correctly print the target directory 🤔 (which seems like the cause of the misbehaviour too)

@hobofan hobofan added C-bug Category: This is a bug C-enhancement Category: This is a new feature labels Mar 13, 2019
@hobofan
Copy link
Member

hobofan commented Mar 13, 2019

Exit gracefully if it can't find the docs.

But yes, the command should definitely handle an nonexistent directory more gracefully, and probably log out a message that recommends using the --dir argument.

Oh boy, I really do spend to little time maintaining this project 😓 . Just saw that I already implemented that in #33, though unreleased as of right now (see #25 for that).

@jyn514
Copy link
Contributor

jyn514 commented Aug 17, 2020

Ran into a similar issue recently, cargo deadlinks can't find the docs with CARGO_TARGET_DIR set to something other than target.

$ cargo doc
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
$ cargo deadlinks
Could not find directory "target/doc/yottadb".

Please run `cargo doc` before running `cargo deadlinks`.
$ echo $CARGO_TARGET_DIR 
/home/joshua/.local/lib/cargo/target

I'm very much against introducing cargo as a dependency though, as it is huge. Kind of surprised that cargo metadata doesn't correctly print the target directory 🤔 (which seems like the cause of the misbehaviour too)

It looks like cargo metadata does print it correctly in this case:

$ cargo metadata | jq .target_directory
"/home/joshua/.local/lib/cargo/target"

@jyn514 jyn514 added the A-cargo Area: Determining directories based on `cargo metadata` label Nov 17, 2020
@jyn514
Copy link
Contributor

jyn514 commented Nov 18, 2020

I would prefer not to handle this in deadlinks itself. I'd have to handle CARGO_BUILD_TARGET, .cargo/config, ~/.cargo/config, $CARGO_HOME ... I'd much rather get this info from cargo directly: rust-lang/cargo#8791

@jyn514 jyn514 added the S-blocked Status: Blocked on another project label Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo Area: Determining directories based on `cargo metadata` C-bug Category: This is a bug C-enhancement Category: This is a new feature S-blocked Status: Blocked on another project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants