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.

Sign up for GitHub

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

Jump to bottom

Changed /sys/kernel/address_bits to /sys/kernel/profiling in test_cp #6294

Merged
merged 3 commits into from
May 2, 2024
Merged

Changed /sys/kernel/address_bits to /sys/kernel/profiling in test_cp #6294

merged 3 commits into from
May 2, 2024

Conversation

AnirbanHalder654322
Copy link
Contributor

/sys/kernel/address_bits was introduced in #6220 and is not present in older distros according to #6276 .

This should fix it.

@AnirbanHalder654322 AnirbanHalder654322 marked this pull request as draft April 30, 2024 16:26
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre force-pushed the replace_testing_virtual_file branch from a565508 to 7bcdc68 Compare April 30, 2024 16:42
@BenWiederhake
Copy link
Collaborator

Do you know when in which Linux kernel versions /sys/kernel/profiling and /sys/kernel/address_bits were added, and when these versions were released? After all, it wouldn't be a good idea if this lowers the "Minimum Supported Linux Version" only by two months or so, because then #6276 will be re-opened by another person soon afterwards.

sylvestre
sylvestre reviewed Apr 30, 2024
View reviewed changes
tests/by-util/test_cp.rs Outdated
@@ -3956,7 +3956,7 @@ fn test_cp_default_virtual_file() {
let ts = TestScenario::new(util_name!());
let at = &ts.fixtures;
ts.ucmd()
.arg("/sys/kernel/address_bits")
.arg("/sys/kernel/profiling")
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a check like
if exits /sys/kernel/address_bits then
/sys/kernel/address_bits
else
/sys/kernel/profiling

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? I assumed that /sys/kernel/profiling was chosen by Anirban because it is supported by "all" Linux versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

is it for real ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the assumption is that /sys/kernel/profiling is supported by almost all linux versions . It should be fine to totally just replace /sys/kernel/address_bits with it.

At the moment I am looking into the changelogs to find out when it was actually added to make sure the assumption actually holds and we don't get a repeat of the same issue few months later.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, sounds good :)

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre force-pushed the replace_testing_virtual_file branch from 7bcdc68 to 3fe701c Compare May 1, 2024 08:10
@github-actions GitHub Actions
Copy link

github-actions bot commented May 1, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre force-pushed the replace_testing_virtual_file branch from 3fe701c to e0540f8 Compare May 2, 2024 13:46
@github-actions GitHub Actions
Copy link

github-actions bot commented May 2, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/timeout/timeout is no longer failing!

BenWiederhake
BenWiederhake requested changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Hi! The choice of the file looks good, please add a comment so that future readers will immediately know why that file was chosen. (Especially in case there's some other problem and we need to pick yet another file.)

Also, please take the PR out of draft-mode before pushing, so that all checks run.

tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
tests/by-util/test_cp.rs Show resolved Hide resolved
tests/by-util/test_cp.rs Show resolved Hide resolved
@AnirbanHalder654322 AnirbanHalder654322 force-pushed the replace_testing_virtual_file branch from e0540f8 to 9dc85d7 Compare May 2, 2024 15:34
@AnirbanHalder654322 AnirbanHalder654322 marked this pull request as ready for review May 2, 2024 15:41
@AnirbanHalder654322
Copy link
Contributor Author

Did i make a mistake force pushing my formatting commit ? Seems like all of the lints keep failing .

@cakebaker
Copy link
Contributor

@AnirbanHalder654322 those clippy errors are unrelated to your PR, they fail because there was a new Rust release today. It's addressed in #6330.

BenWiederhake
BenWiederhake approved these changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

LGTM! Let's see whether any CI platform is missing this file, then this can be merged (or more likely: squashed)

@github-actions GitHub Actions
Copy link

github-actions bot commented May 2, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/timeout/timeout is no longer failing!

@BenWiederhake
Copy link
Collaborator

Looks like all CI failures are due to other things:

In other words: CI is as green as it can be, given the current circumstances.

@BenWiederhake BenWiederhake merged commit c5a530f into uutils:main May 2, 2024
62 of 68 checks passed
@BenWiederhake
Copy link
Collaborator

TIL: The "Squash and merge" commits generated by Github are ugly as hell. Lesson learned, I'll take more care to edit the message next time.

@BenWiederhake BenWiederhake mentioned this pull request May 2, 2024
Closed
@jtracey jtracey mentioned this pull request May 2, 2024
Closed
@BrewTestBot BrewTestBot mentioned this pull request Jun 23, 2024
Merged
@moonfruit moonfruit mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

@BenWiederhake BenWiederhake BenWiederhake approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AnirbanHalder654322 @BenWiederhake @cakebaker @sylvestre