Skip to content

run_make_support: rectify symlink handling - #130427

Merged
bors merged 2 commits into
rust-lang:masterfrom
jieyouxu:rmake-symlink
Sep 17, 2024
Merged

run_make_support: rectify symlink handling#130427
bors merged 2 commits into
rust-lang:masterfrom
jieyouxu:rmake-symlink

Conversation

@jieyouxu

@jieyouxujieyouxu commented Sep 16, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Avoid confusing Unix symlinks and Windows symlinks. Since their
semantics are quite different, we should avoid trying to make it
automagic in how symlinks are created and deleted. Notably, the tests
should reflect what type of symlinks are to be created to match what std
does to make it less surprising for test readers.

@rustbotrustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2024
@jieyouxu

This comment was marked as outdated.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2024
@bors
Auto merge ofrust-lang#130427- jieyouxu:rmake-symlink, r=<try>
48f978b
run_make_support: rectify symlink handling
Avoid confusing Unix symlinks and Windows symlinks, and since their
semantics are quite different we should avoid trying to make it to
automagic in how symlinks are created and deleted. Notably, the tests
should reflect what type of symlinks are to be created to match what std
does to make it less surprising for test readers.
r? `@ghost`
try-job: x86_64-msvc
try-job: aarch64-apple
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
@borsbors 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 Sep 16, 2024
Kobzol
Kobzol reviewed Sep 16, 2024
View reviewed changes
Comment threadtests/run-make/symlinked-extern/rmake.rs Outdated
}
#[cfg(windows)]
{
rfs::windows::symlink_file(

@KobzolKobzolSep 16, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems quite cumbersome, error-prone and hard to read. Can't we create more precise variants of the symlink functions (symlink_file, symlink_dir - or whatever other versions needed) and then let each runmake test decide which exact variant it needs, and handle the implementation difference itself in rfs?

The test will probably always do the same thing on Unix and Windows; we just need to force the test to be more explicit in what kind of symlink it needs.

Like this:

// runmake test
rfs::symlink_file("a","b");
rfs::symlink_dir("x","y");// rfs
unix::symlink_file => symlink
unix::symlink_dir => symlink
windows::symlink_file => symlink_file
windows::symlink_dir => symlink_dir

@jieyouxujieyouxuSep 16, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah that seems reasonable. I'll change this tomorrow.

@jieyouxujieyouxuSep 16, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

(I lied I changed this today)

@jieyouxu
jieyouxu marked this pull request as ready for review September 16, 2024 11:50
@jieyouxu

jieyouxu commented Sep 16, 2024

Copy link
Copy Markdown
MemberAuthor

Should be ready for review now.

r? @Kobzol since you're already looking at it

ChrisDenton
ChrisDenton reviewed Sep 16, 2024
View reviewed changes
Comment threadsrc/tools/run-make-support/src/fs.rs Outdated
Comment threadsrc/tools/run-make-support/src/fs.rs
@jieyouxu
run_make_support: rectify symlink handling
1b86340
Avoid confusing Unix symlinks and Windows symlinks, and since their
semantics are quite different we should avoid trying to make it to
automagic in how symlinks are created and deleted. Notably, the tests
should reflect what type of symlinks are to be created to match what std
does to make it less surprising for test readers.
Kobzol
Kobzol reviewed Sep 16, 2024
View reviewed changes

@KobzolKobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left one nit, otherwise LGTM. Is symlink_metadata intended for future usage or are you planning to modify more tests in this PR?

Comment threadtests/run-make/symlinked-libraries/rmake.rs Outdated
Comment threadtests/run-make/symlinked-extern/rmake.rs Outdated
@jieyouxu

jieyouxu commented Sep 16, 2024

Copy link
Copy Markdown
MemberAuthor

symlink_metadata is intended for future usage, but if we have metadata and support symlinks, then we do need to provide a way to interrogate metadata on a given file without potentially traversing a symlink.

@jieyouxu

jieyouxu commented Sep 16, 2024

Copy link
Copy Markdown
MemberAuthor

Fixed the tests to use path(). Locally ./x test run-make and ./x doc run-make-support both seem to pass on msvc (well except the ones that still use make).

@Kobzol

Kobzol commented Sep 16, 2024

Copy link
Copy Markdown
Member

Good :) You can r=me once CI is green.

@jieyouxu

jieyouxu commented Sep 17, 2024

Copy link
Copy Markdown
MemberAuthor

CI is green, so
@bors r=@Kobzol rollup

@bors

bors commented Sep 17, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 7d76428 has been approved by Kobzol

It is now in the queue for this repository.

@borsbors 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 Sep 17, 2024
@matthiaskrgrmatthiaskrgr mentioned this pull request Sep 17, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
@bors
Auto merge ofrust-lang#130456- matthiaskrgr:rollup-h2qvk1f, r=matth…
c8dff28
…iaskrgr
Rollup of 4 pull requests
Successful merges:
- rust-lang#130380 (coverage: Clarify some parts of coverage counter creation)
- rust-lang#130427 (run_make_support: rectify symlink handling)
- rust-lang#130447 (rustc_llvm: update for llvm/llvm-project@2ae968a0d9fb61606b020e898d88…)
- rust-lang#130448 (fix: Remove duplicate `LazyLock` example.)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 07ca905 into rust-lang:masterSep 17, 2024
@rustbotrustbot added this to the 1.83.0 milestone Sep 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
@rust-timer
Unrolled build forrust-lang#130427
c5db5b5
Rollup merge of rust-lang#130427 - jieyouxu:rmake-symlink, r=Kobzol
run_make_support: rectify symlink handling
Avoid confusing Unix symlinks and Windows symlinks. Since their
semantics are quite different, we should avoid trying to make it
automagic in how symlinks are created and deleted. Notably, the tests
should reflect what type of symlinks are to be created to match what std
does to make it less surprising for test readers.
@jieyouxu
jieyouxu deleted the rmake-symlink branch September 17, 2024 06:30
@jieyouxujieyouxu mentioned this pull request Oct 9, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrisDentonChrisDentonChrisDenton left review comments
@KobzolKobzolKobzol left review comments

Assignees

@KobzolKobzol

Labels

A-run-makeArea: port run-make Makefiles to rmake.rsS-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

No open projects
Oxidizing run-make tests
Status: Done

Milestone

1.83.0

Development

Successfully merging this pull request may close these issues.

6 participants

@jieyouxu@bors@rust-log-analyzer@Kobzol@ChrisDenton@rustbot