Improve macOS and BSD portability#83
Conversation
Replace GNU getopt with manual argument parsing since BSD getopt on macOS doesn't support long options. Change du -b to du -k for portability as the bytes flag is only available in GNU coreutils. Fix shell quoting issues throughout: use "$@" instead of $* for proper handling of arguments with spaces, and quote all variable references to prevent word splitting. Adjust monitor timeout from 360 to 10240 to reflect kilobytes instead of bytes with du -k. These changes ensure the bootstrap script works correctly on both macOS and Linux systems.
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@marcmengel I have verified that the bootstrap itself completes successfully on macOS, but I have not tested that the resulting installation has everything functioning correctly. I'd be happy to receive any suggestions for a concrete set of tests that should be part of the verification. |
|
So a few quick tests:
That should tell if the bootstrap is doing its thing. I would tell you to spack install something from one of the SciSoft buildacaches, but I don't think we have much there for MacOS at the moment... |
| while : | ||
| do | ||
| local duf=$(du -b $f) | ||
| # du -k (kilobytes) is portable; du -b (bytes) is GNU coreutils only and |
There was a problem hiding this comment.
Do you think instead of switching from du -b to du -k, we could go with maybe
wc -c or something that actually gives us an answer in bytes? We're using this to compute our completion percentage, and having the resolution be in kilobytes seems like it will update less often and make it look like the script is stuck.
| echo $* >&3 | ||
| start_monitor $logfile 360 | ||
| # "$@" instead of $* to correctly handle arguments containing spaces. | ||
| echo "$@" >&3 |
There was a problem hiding this comment.
Mutter. we're echoing this to the user, I don't think the whitespace matters... but fine. You asked the tool to find portabiltiy issues, and it kind-of did...
bootstrap: add get_file_size() helper that uses BSD stat on macOS and GNU stat on Linux, replacing du -k which is only accurate in kilobytes. Update progress monitor to work in bytes, correcting the expected size constant to 10485760. make_spack: replace date --iso-8601=seconds (GNU-only) with a portable date format string. Replace getopt --longoptions (GNU-only) with a manual while/case loop compatible with BSD getopt on macOS. Add error messages for unknown options.
|
I think I have to put this PR on pause for now, because it is becoming more work than I can afford. Testing that the updated bootstrap works is hard because part of the bootstrap is downloading other scripts from the repository -- but they download the wrong ones, because the PR isn't merged. There are several parts to tweak to make this testable, so I'll have to put it aside for now. |
Replace GNU getopt with manual argument parsing since BSD getopt on macOS doesn't support long options. Change du -b to du -k for portability as the bytes flag is only available in GNU coreutils.
Fix shell quoting issues throughout: use "$@" instead of $* for proper handling of arguments with spaces, and quote all variable references to prevent word splitting. Adjust monitor timeout from 360 to 10240 to reflect kilobytes instead of bytes with du -k.
These changes ensure the bootstrap script works correctly on both macOS and Linux systems.