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

Uplift .dSYM for unit tests as well #7960

Closed
jiegec opened this issue Mar 3, 2020 · 6 comments
Closed

Uplift .dSYM for unit tests as well #7960

jiegec opened this issue Mar 3, 2020 · 6 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@jiegec
Copy link
Contributor

jiegec commented Mar 3, 2020

Describe the problem you are trying to solve

Currently, debug symbols are uplifted for binaries and examples only, see

// See rust-lang/cargo#4490, rust-lang/cargo#4960.
// Only uplift debuginfo for binaries.
// - Tests are run directly from `target/debug/deps/` with the
// metadata hash still in the filename.
// - Examples are only uplifted for apple because the symbol file
// needs to match the executable file name to be found (i.e., it
// needs to remove the hash in the filename). On Windows, the path
// to the .pdb with the hash is embedded in the executable.
let is_apple = target_triple.contains("-apple-");
if *kind == TargetKind::Bin || (*kind == TargetKind::ExampleBin && is_apple) {

Related Issue: #4490.

Yet it seems that the path in executable key in cargo json format output points to target/debug/xxx-hash instead of target/debug/deps/xxx-hash, so tools like Intellij Rust fail to debug these tests: intellij-rust/intellij-rust#3680.

Cargo json output:

{
    "reason": "compiler-artifact",
    "package_id": "project 0.1.0 (path+file:///path/to/project)",
    "target": {
        "kind": [
            "lib"
        ],
        "crate_types": [
            "lib"
        ],
        "name": "project",
        "src_path": "/path/to/project/src/lib.rs",
        "edition": "2018",
        "doctest": true
    },
    "profile": {
        "opt_level": "0",
        "debuginfo": 2,
        "debug_assertions": true,
        "overflow_checks": true,
        "test": true
    },
    "features": [

    ],
    "filenames": [
        "/path/to/project/target/debug/project-123462f2a057f680"
    ],
    "executable": "/path/to/project/target/debug/project-123462f2a057f680",
    "fresh": true
}

Only /path/to/project/target/debug/deps/project-123462f2a057f680.dSYM exists, but /path/to/project/target/debug/project-123462f2a057f680.dSYM doesn't. So lldb can't pick up debug symbols when debugging /path/to/project/target/debug/project-123462f2a057f680.

Describe the solution you'd like

I think there are three possible solutions:

  1. Change the executable path in JSON to /path/to/project/target/debug/deps/project-123462f2a057f680
  2. Add symlink
  3. Let those tools handle this specific case

Notes

Related issue: rust-lang/rust#59907

@jiegec jiegec added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Mar 3, 2020
@jiegec
Copy link
Contributor Author

jiegec commented Mar 3, 2020

A comprehensive description on this issue is posted by @kwhrtsk at https://github.com/kwhrtsk/rust-lldb-workaround.

@ehuss
Copy link
Contributor

ehuss commented Mar 3, 2020

I don't know the history of how or why things work this way. It looks like 1.0 placed the binary directly into target/debug/ and ran directly from there. Then, in 1.12 it changed to executing directly out of deps/, but it still linked it into the parent (via #2919). It doesn't look like there was any discussion about tests. I can't imagine why tests still needs to be linked to target/debug/ if those filenames are inherently un-discoverable without the JSON output.

One argument for removing the link is that some tests make assumptions about the directory they run from (like this). That might be an argument that tools should only execute from deps/ to match the behavior of Cargo?

Can you (or @alexcrichton) think of any reason it would be bad to stop linking tests into target/debug/?

@alexcrichton
Copy link
Member

What you pointed out is the only thing that I can think of, by changing where we execute tests we change env::current_exe() which might affect programs relying on their location to find sibling binaries. Recently we added env vars for this though so they should be switching to those env vars regardless.

IMO we should have the flexibility to do w/e we want (in general) with the target directory to fix issues like this, it's just a matter of how to best get there. We may have to wait for the env vars to propagate to stable before we make a change to not uplift tests.

@ehuss
Copy link
Contributor

ehuss commented Mar 3, 2020

by changing where we execute tests

To be clear, I'm not suggesting changing where it is executed. Currently Cargo executes it out of target/debug/deps/, but the JSON message says the artifact is in target/debug/. I'm suggesting to change the JSON, and to potentially just stop linking to target/debug/ altogether, since nothing should be relying on it (if we change the JSON path).

@alexcrichton
Copy link
Member

Oh dear my bad, then yeah I can't think of any reason to not change the JSON and stop uplifting the tests.

bors added a commit that referenced this issue Mar 5, 2020
Don't create hardlink for library test and integrations tests, fixing #7960

Related issue: #7960

Problem:

Tests are run under deps, but it is still copied to its parent directory. It leads to separation between the executable and its debug symbols (.dSYM directory).

Solution:

Set hardlink to None.
@jiegec jiegec closed this as completed Mar 5, 2020
@jiegec
Copy link
Contributor Author

jiegec commented Mar 5, 2020

Done in #7965

bors added a commit to rust-lang/rust that referenced this issue Mar 18, 2020
Update cargo

Update cargo

21 commits in bda50510d1daf6e9c53ad6ccf603da6e0fa8103f..7019b3ed3d539db7429d10a343b69be8c426b576
2020-03-02 18:05:34 +0000 to 2020-03-17 21:02:00 +0000
- Run through clippy (rust-lang/cargo#8015)
- Fix config profiles using "dev" in `cargo test`. (rust-lang/cargo#8012)
- Run CI on all PRs. (rust-lang/cargo#8011)
- Add unit-graph JSON output. (rust-lang/cargo#7977)
- Split workspace/validate() into multiple functions (rust-lang/cargo#8008)
- Use Option::as_deref (rust-lang/cargo#8005)
- De-duplicate edges (rust-lang/cargo#7993)
- Revert "Disable preserving mtimes on archives" (rust-lang/cargo#7935)
- Close the front door for clippy but open the back (rust-lang/cargo#7533)
- Fix CHANGELOG.md typos (rust-lang/cargo#7999)
- Update changelog note about crate-versions flag. (rust-lang/cargo#7998)
- Bump to 0.45.0, update changelog (rust-lang/cargo#7997)
- Bump libgit2 dependencies (rust-lang/cargo#7996)
- Avoid buffering large amounts of rustc output. (rust-lang/cargo#7838)
- Add "Updating" status for git submodules. (rust-lang/cargo#7989)
- WorkspaceResolve: Use descriptive lifetime label. (rust-lang/cargo#7990)
- Support old html anchors in manifest chapter. (rust-lang/cargo#7983)
- Don't create hardlink for library test and integrations tests, fixing rust-lang/cargo#7960 (rust-lang/cargo#7965)
- Partially revert change to filter debug_assertions. (rust-lang/cargo#7970)
- Try to better handle restricted crate names. (rust-lang/cargo#7959)
- Fix bug with new feature resolver and required-features. (rust-lang/cargo#7962)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

No branches or pull requests

3 participants