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

Migrate reproducible-build run-make test to rmake #128456

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

Oneirical
Copy link
Contributor

@Oneirical Oneirical commented Jul 31, 2024

Part of #121876 and the associated Google Summer of Code project.

This will likely fail. Locally, rustc errors with linker 'linker' not found on line 36 while the file exists according to the dir-debug statement before it.

If this gets fixed and the test passes, further developments may include:

  • There may be some leftovers from each test - test_in_tmpdir may therefore be required.
  • Try jobs on all ignored architectures.
  • A potential refactor with a struct and a custom function like Migrate remap-path-prefix-dwarf run-make test to rmake #128410 so this isn't just a huge stream of rfs and rustc. This is a little bit harder to do in this test considering the variability present in each test case.

// try-job: x86_64-msvc // windows jobs passed in a prior run
// try-job: x86_64-mingw
// try-job: i686-msvc
// try-job: i686-mingw
try-job: aarch64-apple
try-job: aarch64-gnu
try-job: armhf-gnu
try-job: test-various
try-job: dist-various-1

@rustbot
Copy link
Collaborator

rustbot commented Jul 31, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 31, 2024
@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 1, 2024

Note to self: look at linker not found

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2024
@bors
Copy link
Contributor

bors commented Aug 5, 2024

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

@Oneirical
Copy link
Contributor Author

Looks like the linker problem got fixed, but now, there are some issues with the bin copying on this test:

    run_in_tmpdir(|| {
        rustc().input("reproducible-build-aux.rs").run();
        rfs::create_dir("test");
        rfs::copy("reproducible-build.rs", "test/reproducible-build.rs");
        rustc()
            .input("reproducible-build.rs")
            .crate_type("bin")
            .arg(&format!("--remap-path-prefix={}=/b", cwd().display()))
            .run();
        eprintln!("{:#?}", rfs::shallow_find_dir_entries(cwd()));
        rfs::copy(bin_name("reproducible_build"), bin_name("foo"));
        rustc()
            .input("test/reproducible-build.rs")
            .crate_type("bin")
            .arg("--remap-path-prefix=/test=/b")
            .run();
        assert_eq!(rfs::read(bin_name("reproducible_build")), rfs::read(bin_name("foo")));
    });

I think it might be assuming that the bins are directories due to the lack of extension. I tried manually adding extensions with .output, but no luck, it simply causes the final assertion to fail. I'm letting this one sit for a bit.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 6, 2024

I think it might be assuming that the bins are directories due to the lack of extension. I tried manually adding extensions with .output, but no luck, it simply causes the final assertion to fail. I'm letting this one sit for a bit.

Oh this one is easy. rustc performs hyphen normalization for library names, i.e. reproducible-build.rs hyphen-normalizes to libreproducible_build.rlib or whatever unless you specify output artifact names. But bin names are not hyphen-normalized, so it's actually bin_name("reproducible-build") not bin_name("reproducible_build").

tests/run-make/reproducible-build/rmake.rs Outdated Show resolved Hide resolved
tests/run-make/reproducible-build/rmake.rs Outdated Show resolved Hide resolved
tests/run-make/reproducible-build/rmake.rs Outdated Show resolved Hide resolved
tests/run-make/reproducible-build/rmake.rs Outdated Show resolved Hide resolved
@Oneirical
Copy link
Contributor Author

Oneirical commented Aug 6, 2024

Alright, I have done a big cleanup. This should be a lot nicer to look at. Note this line:

// FIXME(Oneirical): this specific case fails the final assertion.
// diff_dir_test(CrateType::Bin, RemapType::Cwd { is_empty: false });

In the original test:

# TODO: Builds of `bin` crate types are not deterministic with debuginfo=2 on
# Windows.
# See: https://github.com/rust-lang/rust/pull/87320#issuecomment-920105533
# Issue: https://github.com/rust-lang/rust/issues/88982
#
#	different_source_dirs_bin \
#	remap_cwd_bin \

Note that despite the comment mentioning a Windows-specific problem, remap_cwd_bin fails on Linux too, and this part of the test was getting ignored on all architectures, since it's commented out. It's possible that it never worked at all.

As for different_source_dirs_bin, it succeeds, but it might fail on a different architecture. Some testing required. I'd wager it's because the original test didn't check for .exe extensions, though.

Feel free to run try jobs after CI is green.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2024
@Oneirical Oneirical marked this pull request as ready for review August 6, 2024 16:44
@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@jieyouxu
Copy link
Member

jieyouxu commented Aug 7, 2024

Note that despite the comment mentioning a Windows-specific problem, remap_cwd_bin fails on Linux too, and this part of the test was getting ignored on all architectures, since it's commented out. It's possible that it never worked at all.

That seems like the case. If I drop the -g a.k.a. -C debuginfo=2 flag, then remap_cwd_bin passes on linux.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, this is now much easier to follow!

tests/run-make/reproducible-build/rmake.rs Outdated Show resolved Hide resolved
tests/run-make/reproducible-build/rmake.rs Show resolved Hide resolved
tests/run-make/reproducible-build/rmake.rs Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Member

jieyouxu commented Aug 7, 2024

@bors try

@jieyouxu
Copy link
Member

jieyouxu commented Aug 7, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2024
…<try>

Migrate `reproducible-build` `run-make` test to rmake

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This will likely fail. Locally, rustc errors with `linker 'linker' not found` on line 36 while the file exists according to the dir-debug statement before it.

If this gets fixed and the test passes, further developments may include:

- [x] There may be some leftovers from each test - `test_in_tmpdir` may therefore be required.
- [ ] Try jobs on all ignored architectures.
- [x] A potential refactor with a struct and a custom function like rust-lang#128410 so this isn't just a huge stream of `rfs` and `rustc`. This is a little bit harder to do in this test considering the variability present in each test case.

try-job: aarch64-apple
try-job: test-various
try-job: armhf-gnu
try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-gnu-llvm-17
try-job: dist-various-1
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 15, 2024
@jieyouxu
Copy link
Member

Trying again.
@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
…<try>

Migrate `reproducible-build` `run-make` test to rmake

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This will likely fail. Locally, rustc errors with `linker 'linker' not found` on line 36 while the file exists according to the dir-debug statement before it.

If this gets fixed and the test passes, further developments may include:

- [x] There may be some leftovers from each test - `test_in_tmpdir` may therefore be required.
- [ ] Try jobs on all ignored architectures.
- [x] A potential refactor with a struct and a custom function like rust-lang#128410 so this isn't just a huge stream of `rfs` and `rustc`. This is a little bit harder to do in this test considering the variability present in each test case.

// try-job: aarch64-apple
// try-job: aarch64-gnu
// try-job: armhf-gnu
// try-job: test-various
// try-job: dist-various-1
try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
@bors
Copy link
Contributor

bors commented Aug 15, 2024

⌛ Trying commit b80db63 with merge f3c4813...

@bors
Copy link
Contributor

bors commented Aug 15, 2024

☀️ Try build successful - checks-actions
Build commit: f3c4813 (f3c48134e356f29e7db8f2d40855e48d467749f1)

Copy link
Member

@jieyouxu jieyouxu 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 nit for an outdated FIXME, then r=me after a rebase and running the second batch of non-Windows try-jobs and if they come back green.

Comment on lines 22 to 27
// FIXME(Oneirical): ignore-windows
// # FIXME: Builds of `bin` crate types are not deterministic with debuginfo=2 on
// # Windows.
// # See: https://github.com/rust-lang/rust/pull/87320#issuecomment-920105533
// # Issue: https://github.com/rust-lang/rust/issues/88982
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: this comment is moved to specific cases, we should remove this top-level ignore-windows FIXME comment.

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2024
@Oneirical
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 16, 2024

⌛ Trying commit e752410 with merge 3d3742d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2024
…<try>

Migrate `reproducible-build` `run-make` test to rmake

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This will likely fail. Locally, rustc errors with `linker 'linker' not found` on line 36 while the file exists according to the dir-debug statement before it.

If this gets fixed and the test passes, further developments may include:

- [x] There may be some leftovers from each test - `test_in_tmpdir` may therefore be required.
- [ ] Try jobs on all ignored architectures.
- [x] A potential refactor with a struct and a custom function like rust-lang#128410 so this isn't just a huge stream of `rfs` and `rustc`. This is a little bit harder to do in this test considering the variability present in each test case.

// try-job: x86_64-msvc // windows jobs passed in a prior run
// try-job: x86_64-mingw
// try-job: i686-msvc
// try-job: i686-mingw
try-job: aarch64-apple
try-job: aarch64-gnu
try-job: armhf-gnu
try-job: test-various
try-job: dist-various-1
@bors
Copy link
Contributor

bors commented Aug 16, 2024

☀️ Try build successful - checks-actions
Build commit: 3d3742d (3d3742d2207d10c64423b2425b4840ef23eb55c4)

@Oneirical
Copy link
Contributor Author

@bors r=@jieyouxu

@bors
Copy link
Contributor

bors commented Aug 16, 2024

📌 Commit e752410 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2024
@jieyouxu
Copy link
Member

@bors rollup=iffy (reproducible-build isn't always reproducible)

@bors
Copy link
Contributor

bors commented Aug 16, 2024

⌛ Testing commit e752410 with merge 569d7e3...

@bors
Copy link
Contributor

bors commented Aug 16, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 569d7e3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 16, 2024
@bors bors merged commit 569d7e3 into rust-lang:master Aug 16, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 16, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (569d7e3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.0% [-1.8%, -0.2%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.5%, secondary 0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
1.7% [1.1%, 2.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.8% [-0.8%, -0.8%] 1
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Cycles

Results (secondary -3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-4.5%, -2.1%] 14
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 751.349s -> 750.018s (-0.18%)
Artifact size: 337.10 MiB -> 339.17 MiB (0.61%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants