extend `detect_src_and_out` test #109162

onur-ozkan · 2023-03-15T12:06:57Z

I was thinking about the following cases when I wrote the comment in #109055

Running bootstrap from the source root.

Running from a subdirectory of the source root.

Running from outside the source root.

Running on a different machine from where bootstrap was compiled (which will be important > for no python in shell scripts #107812). You can mostly replicate this by renaming the source root so it no longer exists on disk.

Running with --build-dir.

Running with $RUST_BOOTSTRAP_CONFIG set in the environment and build-dir set in the file.

Tested all the topics mentioned above. All worked fine. The test is now also covers if build dir is manually specified in config.

r? @jyn514

helps #109120 partially

jyn514 · 2023-03-18T20:45:00Z

This change looks good to me, but I'm somewhat confused by the PR description? I only see tests for build-dir, not the other things mentioned in the issue.

Anyway, r=me on this change, but please remove "fixes" from the PR description if you want to merge it before I take another look.

onur-ozkan · 2023-03-18T20:51:20Z

This change looks good to me, but I'm somewhat confused by the PR description? I only see tests for build-dir, not the other things mentioned in the issue.

Anyway, r=me on this change

https://github.com/rust-lang/rust/blob/5f01f7bbd507641ccc83c63a8b7414ff00ca5356/src/bootstrap/config/tests.rs#L68-L76

This already covers first 4 topics you mentioned. I performed all the scenarios and test passed without any trouble.

but please remove "fixes" from the PR description if you want to merge it before I take another look.

You mean resolves #109120 part?

I will take another look and see if I can extend it even more.

onur-ozkan · 2023-03-19T11:32:09Z

Running bootstrap from the source root.

Running from a subdirectory of the source root.

Running from outside the source root.

Running on a different machine from where bootstrap was compiled (which will be important > for no python in shell scripts #107812). You can mostly replicate this by renaming the source root so it no longer exists on disk.

These should already be covered because we check build artifacts and environments that are being applied from bootstrap. So executing bootstrap from any directory or with any --build-dir shouldn't make difference.

Running with --build-dir.

Running with $RUST_BOOTSTRAP_CONFIG set in the environment and build-dir set in the file.

We have test for that

https://github.com/rust-lang/rust/blob/5f01f7bbd507641ccc83c63a8b7414ff00ca5356/src/bootstrap/config/tests.rs#L81

@jyn514 I am not sure how far this test can be extended, I guess you have so more ideas. Feel free to remove resolves part from PR description if you think this test can be extended more.

out of context: I extended testing src part a little more by adding CARGO_MANIFEST_DIR check.

onur-ozkan · 2023-03-19T11:32:16Z

@rustbot ready

jyn514 · 2023-04-05T00:58:24Z

So executing bootstrap from any directory or with any --build-dir shouldn't make difference.

This is what makes me nervous - the test relies on being run in the scenarios I've mentioned to be effective (i.e. after your change x test bootstrap from outside the source root will only succeed if the src/out logic is correct). But CI never runs x test bootstrap from outside the source root. So this is still only catching issues once someone, usually a distro, sets a custom config. I would like to also change CI to run x test bootstrap in the scenarios I've mentioned.

Running on a different machine from where bootstrap was compiled (which will be important > for

This is definitely not being tested, expected_out and expected_src will both be incorrect if the CARGO_MANIFEST_DIR doesn't exist on disk.

onur-ozkan · 2023-04-05T09:23:42Z

@rustbot author

onur-ozkan · 2023-04-06T10:06:41Z

I currently have several Rust tasks assigned to me that take priority. As this version of the test is significantly better than the previous one, I would like to merge it to main and avoid blocking it from other tasks of mine. I have added a FIXME tag with my name on it, and I will return to improving this test at a later time, once I have finished the other tasks assigned to me.

r=me on this change, but please remove "fixes" from the PR description

Updated it.

@bors r=jyn514 rollup

bors · 2023-04-06T10:06:43Z

📌 Commit 38b1741 has been approved by jyn514

It is now in the queue for this repository.

…t_test, r=jyn514 extend `detect_src_and_out` test > I was thinking about the following cases when I wrote the comment in rust-lang#109055 > > 1. Running bootstrap from the source root. > 2. Running from a subdirectory of the source root. > 3. Running from outside the source root. > 4. Running on a different machine from where bootstrap was compiled (which will be important > for rust-lang#107812). You can mostly replicate this by renaming the source root so it no longer exists on disk. > 5. Running with `--build-dir`. > 6. Running with `$RUST_BOOTSTRAP_CONFIG` set in the environment and `build-dir` set in the file. Tested all the topics mentioned above. All worked fine. The test is now also covers if build dir is manually specified in config. r? `@jyn514` helps rust-lang#109120 partially

…t_test, r=jyn514 extend `detect_src_and_out` test > I was thinking about the following cases when I wrote the comment in rust-lang#109055 > > 1. Running bootstrap from the source root. > 2. Running from a subdirectory of the source root. > 3. Running from outside the source root. > 4. Running on a different machine from where bootstrap was compiled (which will be important > for rust-lang#107812). You can mostly replicate this by renaming the source root so it no longer exists on disk. > 5. Running with `--build-dir`. > 6. Running with `$RUST_BOOTSTRAP_CONFIG` set in the environment and `build-dir` set in the file. Tested all the topics mentioned above. All worked fine. The test is now also covers if build dir is manually specified in config. r? ``@jyn514`` helps rust-lang#109120 partially

matthiaskrgr · 2023-04-06T16:41:44Z

@bors r- failed in a rollup #110006

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan · 2023-04-06T17:18:10Z

@bors r=jyn514 rollup

bors · 2023-04-06T17:18:12Z

📌 Commit 925a304 has been approved by jyn514

It is now in the queue for this repository.

jyn514 · 2023-04-06T17:46:31Z

@bors rollup=iffy

bors · 2023-04-06T21:18:53Z

⌛ Testing commit 925a304 with merge 28a2928...

bors · 2023-04-06T23:36:13Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 28a2928 to master...

rust-timer · 2023-04-07T00:52:08Z

Finished benchmarking commit (28a2928): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.4%	[-3.4%, -3.4%]	1

Cycles

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

rustbot assigned jyn514 Mar 15, 2023

rustbot added 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 Mar 15, 2023

jyn514 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 Mar 18, 2023

onur-ozkan force-pushed the extend_detect_src_and_out_test branch from 7e6d1f3 to 5f01f7b Compare March 19, 2023 11:23

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 Mar 19, 2023

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 Apr 5, 2023

onur-ozkan force-pushed the extend_detect_src_and_out_test branch from 5f01f7b to bcf5647 Compare April 6, 2023 09:33

onur-ozkan 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 Apr 6, 2023

This comment has been minimized.

Sign in to view

onur-ozkan force-pushed the extend_detect_src_and_out_test branch from bcf5647 to 38b1741 Compare April 6, 2023 09:44

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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2023

Dylan-DPC mentioned this pull request Apr 6, 2023

Rollup of 7 pull requests #109998

Closed

matthiaskrgr mentioned this pull request Apr 6, 2023

Rollup of 8 pull requests #110006

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2023

improve/extend detect_src_and_out test

925a304

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan force-pushed the extend_detect_src_and_out_test branch from 38b1741 to 925a304 Compare April 6, 2023 17:17

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 Apr 6, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2023

bors merged commit 28a2928 into rust-lang:master Apr 6, 2023
12 checks passed

rustbot added this to the 1.70.0 milestone Apr 6, 2023

onur-ozkan deleted the extend_detect_src_and_out_test branch April 7, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend `detect_src_and_out` test #109162

extend `detect_src_and_out` test #109162

onur-ozkan commented Mar 15, 2023 •

edited

jyn514 commented Mar 18, 2023 •

edited

onur-ozkan commented Mar 18, 2023 •

edited

onur-ozkan commented Mar 19, 2023

onur-ozkan commented Mar 19, 2023

jyn514 commented Apr 5, 2023

onur-ozkan commented Apr 5, 2023

This comment has been minimized.

onur-ozkan commented Apr 6, 2023

bors commented Apr 6, 2023

matthiaskrgr commented Apr 6, 2023

onur-ozkan commented Apr 6, 2023

bors commented Apr 6, 2023

jyn514 commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

rust-timer commented Apr 7, 2023

extend detect_src_and_out test #109162

extend detect_src_and_out test #109162

Conversation

onur-ozkan commented Mar 15, 2023 • edited

jyn514 commented Mar 18, 2023 • edited

onur-ozkan commented Mar 18, 2023 • edited

onur-ozkan commented Mar 19, 2023

onur-ozkan commented Mar 19, 2023

jyn514 commented Apr 5, 2023

onur-ozkan commented Apr 5, 2023

This comment has been minimized.

onur-ozkan commented Apr 6, 2023

bors commented Apr 6, 2023

matthiaskrgr commented Apr 6, 2023

onur-ozkan commented Apr 6, 2023

bors commented Apr 6, 2023

jyn514 commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

rust-timer commented Apr 7, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

extend `detect_src_and_out` test #109162

extend `detect_src_and_out` test #109162

onur-ozkan commented Mar 15, 2023 •

edited

jyn514 commented Mar 18, 2023 •

edited

onur-ozkan commented Mar 18, 2023 •

edited