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

build-gnu.sh: fix for /usr/bin/timeout on MacOS #5194

Merged
merged 13 commits into from
Sep 4, 2023

Conversation

zhitkoff
Copy link
Contributor

@zhitkoff zhitkoff commented Aug 22, 2023

Set to /usr/local/timeout if /usr/bin/timeout is not found.
On MacOS there is no system /usr/bin/timeout and trying to add it to /usr/bin (with symlink of copy binary) will fail unless system integrity protection is disabled (not ideal)
ref: https://support.apple.com/en-us/102149
On MacOS the Homebrew coreutils could be installed and then
sudo ln -s /opt/homebrew/bin/timeout /usr/local/bin/timeout

fixes #5193

@@ -142,6 +151,9 @@ sed -i -e '/tests\/misc\/seq-precision.sh/ D' \
sed -i '/INT_OFLOW/ D' tests/misc/printf.sh

# Use the system coreutils where the test fails due to error in a util that is not the one being tested
# TODO : tests/tail-2/ does not appear to exist
# and have been moved to just tests/tail/ location
# Might need to update the section bvelow to reflect that
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Might need to update the section bvelow to reflect that
# Might need to update the section below to reflect that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! fixed

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Aug 22, 2023

Does it maybe make sense to do the check one time to figure out the timeout command and reuse that?

It's too bad that homebrew needs to be installed. Maybe another possibility is that we could make a cross-platform Python script like this to run on any platform:

import subprocess
import sys

let res = subprocess.run(sys.argv[2:], timeout=float(sys.argv[1]))
print("===stdout===")
print(res.stdout)
print("===stderr===")
print(res.stderr)
sys.exit(res.returncode)

This would be kind of interesting, because that would be the script to run a test, which we could then also expand.

Edit: I'm not necessarily suggesting we do this now, it's more just something we could think about.

@zhitkoff
Copy link
Contributor Author

Does it maybe make sense to do the check one time to figure out the timeout command and reuse that?

it sure does :) ... refactored to check once and reuse that

It's too bad that homebrew needs to be installed.

On MacOS there is no way around it, homebrew has to be installed, otherwise most of this script (and other scripts and tools) would not work at all. It is not that only timeout is missing, but xargs, wget, xz and many others. On top of that - included coreutils like /usr/bin/sed are very old and fail due to not recognizing arguments, so have to replace with latest GNU via homebrew. Here is the list that had to be installed so far on fresh vanilla MacOS Ventura:

brew install coreutils
brew install autoconf
brew install autopoint
brew install gettext
brew install wget
brew install texinfo
brew install xz
brew install automake
brew install gnu-sed
brew install m4
brew install bison
brew install pre-commit
brew install findutils

@tertsdiepraam
Copy link
Member

Here is the list that had to be installed so far on fresh vanilla MacOS Ventura:

Oof that's a long list. Thanks for checking that out. Sadly, I guess there's not much we can do there, because this script is provided by GNU. I guess we could document that list somewhere though.

# TODO: Might need to review and updated sections below
# TODO: As a result this script will fail when executed normally as 'bash util/build-gnu.hs' due to the 'set -e' set at the beginning
# TODO: The rest of the 'sed' commands after first failure in this scenario will not be executed as bash will exit on first error
# TODO: However, the behaviour might be different when running it via GitHub actions (GnuTests)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this has been changed after the last coreutils release. You should be able to fix it by checking out the tag of the latest release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting ... I have branched of from main that was already at release 0.0.20, admittedly not from the release tag, just HEAD on main:
Cargo.toml
[package]
name = "coreutils"
version = "0.0.20"

has there been a regression since that release (it looks like it is latest)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just checked out release by tag 0.0.20 ... the issues are still there starting at line 135 on build-gnu.sh
i.e. the script will stop executing here:
sed -i '/INT_OFLOW/ D' tests/misc/printf.sh

since tests/misc/printf.sh does not exist at main (HEAD) GNU coreutils at 'https:/coreutils/coreutils.git'

what am I missing?

Copy link
Contributor Author

@zhitkoff zhitkoff Aug 26, 2023

Choose a reason for hiding this comment

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

ok, never mind :) what I am missing is that you were talking about checking out latest release tag of GNU coreutils, not this repo. Will update the PR.

On that note: would it make sense to modify the message to indicate that latest release tag needs to be checkout after cloning GNU coreutils?
`
Could not find GNU coreutils (expected at '/gnu')

Run the following to download into the expected path:

git clone --recurse-submodules https:/coreutils/coreutils.git "/gnu"
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR to target correct GNU coreutlis release

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I meant GNU. Sorry gor the confusion 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, thanks! Added GNU coreutlis release tag to the script

@zhitkoff
Copy link
Contributor Author

Considering #5221 PR is in progress to upgrade to GNU/coreutils v9.4 release - should this PR also target v9.4 ? Also, would it make sense to have targeted GNU/coreutils release value in one place only (i.e. only in .github/workflows/GnuTests.yml) ? If so, we can change util/build-gnu.sh to grab it from there instead of hardcoding it in the script.
@sylvestre @tertsdiepraam what would you recommend?

@tertsdiepraam
Copy link
Member

Maybe we should fix the tests for 9.4 first and then rebase this PR? I think it would make sense to separate the two changes, but I don't really mind in which order we merge them.

@tertsdiepraam
Copy link
Member

#5221 has been merged. Could you rebase this PR?

@zhitkoff
Copy link
Contributor Author

@tertsdiepraam rebased and pointed to v9.4

util/build-gnu.sh Outdated Show resolved Hide resolved
@zhitkoff
Copy link
Contributor Author

@sylvestre thanks for catching that typo. Interestingly the Code Spell Checker extension for VSCode does not catch that by default - it has to be manually enabled for shell scripts.
This change to .vscode/cSpell.json fixes it

"enableFiletypes": [
    "shellscript"
  ]

Should it be added to this PR or have a separate small PR for this?

@zhitkoff
Copy link
Contributor Author

zhitkoff commented Sep 4, 2023

@tertsdiepraam @sylvestre with most recent changes - do you feel like it can be merged or there is more work to be done?

@sylvestre sylvestre merged commit 862a2df into uutils:main Sep 4, 2023
46 of 47 checks passed
@sylvestre
Copy link
Contributor

yes, sorry about that :)

@zhitkoff zhitkoff deleted the zhitkoff/issue5193 branch November 18, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build-gnu.sh: /usr/bin/timeout should not be hardcoded to /usr/bin location
3 participants