Include executable in JSON output. #5517

patriksvensson · 2018-05-11T09:02:20Z

rust-highfive · 2018-05-11T09:02:22Z

(rust_highfive has picked a reviewer for you, use r? to override)

patriksvensson · 2018-05-11T09:05:18Z

src/cargo/core/compiler/mod.rs

@@ -393,6 +393,22 @@ fn link_targets<'a, 'cfg>(
        .collect();
    let json_messages = bcx.build_config.json_messages();

+    // Find the executable for the current target (if any).
+    let mut executable : Option<PathBuf> = Option::None;


Wanted to move this inside the job below but ran into problems with Unit. Any suggestions of how to resolve these problems are appreciated. Not sure if there are any side effects with the current approach.

No, this is exactly how it should look like, and it's kinda cool that Rust detects this at compile time. The Work thing which we create below is a long-lived closure, and works are executed concurrently. If in that closure we close over Context, than bad things would happen, because several works would try to access Context simultaneously from different threads, and context is not ready for that.

So Rust enforces that we compute all relevant data before hand, and store just simple stuff like Strings inside the Work.

Also, you can use let mut executable: Option<PathBuf> = None;, no need to qualify None. However, I hope when bindist is extracted as a separate function, this could be simplified to let executable = if produces_executable(unit) { bindist(unit) } else { None }.

@matklad Awesome explanation. Thanks!

patriksvensson · 2018-05-11T09:06:27Z

tests/testsuite/build.rs

@@ -4213,6 +4217,7 @@ fn message_format_json_forward_stderr() {
            "overflow_checks": false,
            "test":false
        },
+        "executable": "{...}",


Not verifying the executable here since that's not what's being tested here. Is that OK?

Yep, that's ok!

patriksvensson · 2018-05-11T12:45:18Z

@matklad Any idea why tests are failing on Linux and MacOS?

Expected part:
"[..]foo[EXE]"
Actual part:
"/home/travis/build/rust-lang/cargo/target/cit/t168/foo/target/debug/foo"

ehuss · 2018-05-11T16:05:14Z

@patriksvensson The [..] match is not greedy, so it is only matching up to the first foo. Something like "[..]foo[/]target[/]debug[/]foo[EXE]" should work.

matklad · 2018-05-11T19:06:28Z

src/cargo/core/compiler/mod.rs

+            Some(ref link_dst) => link_dst,
+            None => &output.path,
+        };
+        if unit.target.is_bin() || unit.target.is_bin_example() {


I think the logic here needs to be adjusted slightly. In Cargo, there are several flavors of excutables:

binaries and examples, where the user writes fn main() themselves

tests and becnhmars, where fn main() is synthesized by the compiler

build scripts

I think we definitely should specify "executable" for 1 and 2, and maybe for 3 as well.

The test setup for it would look like this:

λ exa -GT . ├── benches │ └── benchmark.rs ├── Cargo.toml ├── src │ ├── lib.rs │ └── main.rs └── tests └── integration_test.rs

Then, executing cargo test --no-run should show executables for integration_test and for lib.rs (when testing, libary is compiled as an executable which launches unit-tests).

Executing cargo bench --no-run should show exetable for benchmark.

@matklad I'm having some problems with the tests here. It seems like that the JSON lines emitted by cargo test isn't emitted in a deterministic order. Am I missing something here or might this be a bug?

I think this is ok: for example, Cargo could compile tests and becnhmarks in parallel, so either can be ready first. You could add with_json_unordered method to the testing infra to avoid the problem. It also should be possible to modify the test such that there's only one executable per compilation. For example, you can run cargo build --lib, cargo build --bin separately.

@matklad Ah, didn't know about the with_json_unordered approach 👍 I'm going with separate assert passes like you suggest since that one sounds cleaner somehow. Thanks!

matklad · 2018-05-11T19:08:58Z

src/cargo/core/compiler/mod.rs

+        let bindst = match output.hardlink {
+            Some(ref link_dst) => link_dst,
+            None => &output.path,
+        };


This logic is duplicated when we populate Compilation::binaries. Let's extract a helper function fn exetutable_dest(unit: &Unit)?

It might need access to cx as well.

@matklad Do you mean the let bindst logic?

Yep! The "is this unit executable" logic needs to be different.

matklad · 2018-05-11T19:10:05Z

src/cargo/core/compiler/mod.rs

@@ -439,6 +455,7 @@ fn link_targets<'a, 'cfg>(
                profile: art_profile,
                features,
                filenames: destinations,
+                executable: executable,


Could be just executable: if field name and var name are the same, you don't have to write foo: foo.

matklad · 2018-05-11T19:20:38Z

@patriksvensson the actual implementation looks great to me, though I think we need to tweak a logic of what is considered an executable. Basically, we need to make sure that tests and benches also have not-None executable in the corresponding artifact.

patriksvensson · 2018-05-12T04:19:50Z

@matklad I noticed that for tests (regular one, not integration tests), there is nothing in the metadata about the target that seem to hint about it producing an executable (except the filename when running on Windows).

{
    "executable": null,
    "features": [],
    "filenames": [
        "D:\\Source\\github\\patriksvensson\\cargo\\target\\cit\\t0\\foo\\target\\debug\\foo-3398de0ad2d6876f.exe"
    ],
    "fresh": false,
    "package_id": "foo 0.0.1 (path+file:///D:/Source/github/patriksvensson/cargo/target/cit/t0/foo)",
    "profile": {
        "debug_assertions": true,
        "debuginfo": 2,
        "opt_level": "0",
        "overflow_checks": true,
        "test": true
    },
    "reason": "compiler-artifact",
    "target": {
        "crate_types": [
            "lib"
        ],
        "kind": [
            "lib"
        ],
        "name": "foo",
        "src_path": "D:\\Source\\github\\patriksvensson\\cargo\\target\\cit\\t0\\foo\\src\\lib.rs"
    }
}

Any ideas about how to detect this?

matklad · 2018-05-12T14:00:42Z

@patriksvensson

I believe this logic handles this:

cargo/src/cargo/core/compiler/context/mod.rs

Line 178 in b24b455

if unit.mode.is_any_test() && !unit.mode.is_check() {

patriksvensson · 2018-05-30T13:43:28Z

@matklad Any idea why I get a error: Could not compile 'foo'. error on Linux? Seems to work fine on Windows.

alexcrichton · 2018-07-05T19:08:44Z

ping @patriksvensson, were you able to track down the CI failures?

patriksvensson · 2018-07-05T19:58:10Z

@alexchrichton No, I wasn't. Currently on vacation with family but planning to take another look at it in about 3 days.

alexcrichton · 2018-07-05T20:08:55Z

Sure thing, no worries!

bors · 2018-07-05T20:45:54Z

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

Closes #5426

alexcrichton · 2018-07-13T15:28:44Z

@patriksvensson looks like some tests may still be failing?

patriksvensson · 2018-07-13T15:48:19Z

@alexchrichton Yes, I had some spare time so I rebased my PR against master. Will take a look at the failing tests as soon as I have some spare time again.

alexcrichton · 2018-07-13T16:14:30Z

Ok, no worries!

bors · 2018-07-13T20:09:24Z

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

alexcrichton · 2018-07-30T22:09:31Z

Ok I'm gonna close this for now, but feel free to update and I can always reopen!

@patriksvensson

Include executable in JSON output. Fixes #5426 Rebase of @patriksvensson's #5517 CC @matklad I didn't really get into the issue or the code, I just interatively rebased Patrik's branch and then massaged and cleaned up the code until the tests passed. So please double check it for code correctness, test case correctness and test case coverage. Particularly the branch changed an if condition according to [this suggestion](#5517 (comment)) by Aleksey. I rolled that back because at one point it helped fix a series of tests. But let me know if that should be included here.

rust-highfive assigned matklad May 11, 2018

patriksvensson commented May 11, 2018

View reviewed changes

matklad reviewed May 11, 2018

View reviewed changes

Include executable in JSON output.

2a1b161

Closes #5426

alexcrichton closed this Jul 30, 2018

dwijnand mentioned this pull request Nov 30, 2018

Include executable in JSON output. #6363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include executable in JSON output. #5517

Include executable in JSON output. #5517

patriksvensson commented May 11, 2018

rust-highfive commented May 11, 2018

patriksvensson May 11, 2018

matklad May 11, 2018

matklad May 11, 2018

patriksvensson May 11, 2018 •

edited

Loading

patriksvensson May 11, 2018

matklad May 11, 2018

patriksvensson commented May 11, 2018

ehuss commented May 11, 2018

matklad May 11, 2018

patriksvensson May 27, 2018 •

edited

Loading

matklad May 27, 2018

patriksvensson May 27, 2018

matklad May 11, 2018

matklad May 11, 2018

patriksvensson May 11, 2018

matklad May 11, 2018

matklad May 11, 2018

matklad commented May 11, 2018

patriksvensson commented May 12, 2018

matklad commented May 12, 2018

patriksvensson commented May 30, 2018 •

edited

Loading

alexcrichton commented Jul 5, 2018

patriksvensson commented Jul 5, 2018

alexcrichton commented Jul 5, 2018

bors commented Jul 5, 2018

alexcrichton commented Jul 13, 2018

patriksvensson commented Jul 13, 2018

alexcrichton commented Jul 13, 2018

bors commented Jul 13, 2018

alexcrichton commented Jul 30, 2018

Include executable in JSON output. #5517

Include executable in JSON output. #5517

Conversation

patriksvensson commented May 11, 2018

rust-highfive commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriksvensson May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriksvensson commented May 11, 2018

ehuss commented May 11, 2018

Choose a reason for hiding this comment

patriksvensson May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented May 11, 2018

patriksvensson commented May 12, 2018

matklad commented May 12, 2018

patriksvensson commented May 30, 2018 • edited Loading

alexcrichton commented Jul 5, 2018

patriksvensson commented Jul 5, 2018

alexcrichton commented Jul 5, 2018

bors commented Jul 5, 2018

alexcrichton commented Jul 13, 2018

patriksvensson commented Jul 13, 2018

alexcrichton commented Jul 13, 2018

bors commented Jul 13, 2018

alexcrichton commented Jul 30, 2018

patriksvensson May 11, 2018 •

edited

Loading

patriksvensson May 27, 2018 •

edited

Loading

patriksvensson commented May 30, 2018 •

edited

Loading