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

Add a generic install.sh and update README.md. #88

Merged
merged 42 commits into from
Sep 7, 2024

Conversation

kwlzn
Copy link
Collaborator

@kwlzn kwlzn commented Sep 4, 2024

  • Adds a generic install.sh utility.
  • Updates README.md to use the new installer.
  • Adds tests.

OSX install example:

om:~ kw$ curl -LSsf https://raw.githubusercontent.com/kwlzn/lift/kwlzn/installer/install.sh | /bin/bash
Download URL is: https://github.com/a-scie/lift/releases/latest/download/science-fat-macos-aarch64
######################################################################## 100.0%
######################################################################## 100.0%
Download completed successfully
Download matched it's expected sha256 fingerprint, proceeding
Installed https://github.com/a-scie/lift/releases/latest/download/science-fat-macos-aarch64 to /Users/kw/.local/bin/science
WARNING: /Users/kw/.local/bin is not detected on $PATH
You'll either need to invoke /Users/kw/.local/bin/science explicitly or else add /Users/kw/.local/bin to your shell's PATH.
om:~ kw$ /Users/kw/.local/bin/science -V
0.7.0

@kwlzn kwlzn changed the title Add a generic installer.sh and update README.md. Add a generic install.sh and update README.md. Sep 4, 2024
Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

Thanks for chipping in.

I had left things as advisory - aka I leave it to you to know how to download a thing and check a hash and not tackled the bigger problem of an installer until 1.0. Getting a head start on that is good, but I'd definitely want these things in the end:

  • science.scie.app docs have an install page with instructions / links and the instructions should probably use a sice.app URL (if there were install scripts and they lived in this repo, they could be symlinked into the doc tree so that they get published to the doc site).
  • Install should be at least explained for each supported platform, ideally though, it would be supported via script or officially supported package.

There is roughly that for scie-pants for example between:

I'm not sure what you were gunning for here @kwlzn. A step towards that sort of thing maybe? No matter the intent, a basic CI test on the supported platforms at least would be great. Even if just a step that ran the installer and then ran the installed binary and confirmed the version output.

install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@kwlzn
Copy link
Collaborator Author

kwlzn commented Sep 5, 2024

I'm not sure what you were gunning for here @kwlzn. A step towards that sort of thing maybe?

yeah, a pre-1.0 rough edged step in that direction. essentially, just aiming to make it trivial to link to and have a complete newcomer running science -h (as docs) in <1 minute in case it comes up in integration discussions with other OSS projects.

I'm happy to refine this a bit more tho and port get_pants.sh's approach/logic + add some CI coverage with 1.0 in mind.

@kwlzn kwlzn requested a review from jsirois September 5, 2024 13:26
Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

LGTM, we'll see what CI says.

install.sh Outdated Show resolved Hide resolved
install.sh Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests/test_installer.py Outdated Show resolved Hide resolved
tests/test_installer.py Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
tests/test_installer.py Outdated Show resolved Hide resolved
@kwlzn kwlzn requested a review from jsirois September 6, 2024 00:21
tests/test_installer.py Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@kwlzn
Copy link
Collaborator Author

kwlzn commented Sep 7, 2024

the salient part of the latest CI failure seems to be a failure in the pwsh curl mode call:

pwsh -c \'Invoke-WebRequest -OutFile /c/actions-runner/_work/lift/lift/.nox/f83bbc49/tmp/tmp.LGZBCkLskN/science-fat-windows-x86_64.exe.sha256 -Uri https://github.com/a-scie/lift/releases/download/v0.7.0/science-fat-windows-x86_64.exe.sha256\'
 Invoke-WebRequest: 
 Could not find a part of the path \'C:\\c\\actions-runner\\_work\\lift\\lift\\.nox\\f83bbc49\\tmp\\tmp.LGZBCkLskN\\science-fat-windows-x86_64.exe.sha256\'.

my intuition says the /c/... in /c/actions-runner/... (from mktemp -d) implies the C:\ bit in alternate form, but pwsh is prepending to that to arrive at C:\c\.... need to do some more digging.

@jsirois
Copy link
Contributor

jsirois commented Sep 7, 2024

I think probably not. It seems highly likely dirname / basename don't play well with \.

@kwlzn
Copy link
Collaborator Author

kwlzn commented Sep 7, 2024

I think probably not. It seems highly likely dirname / basename don't play well with .

afaict, basename is used exactly once, with a URL and not a filepath, so that's natively using / always and spits out a single path part sans any path separators. and dirname is only ever called on OSX as a x-platform way to net install -D behavior. almost all of the logic leading up to this point is lifted directly from get-pants.sh, which presumably is known to work on Windows - which is the most baffling part.

I did notice one mixed \ + / path construction in the script in debug logging (due to -d receiving a windows temp dir path from the TempPathFactory fixture), but already fixed that in baba4f4.

there is one other similar path separator join (directly adjacent to where basename is used), but that's only ever joined with a mktemp -d output which appears to use / exclusively on windows fwict.

@kwlzn
Copy link
Collaborator Author

kwlzn commented Sep 7, 2024

explicitly converting the /c/x/y/z mktemp-returned MSYS POSIX format to C:\x\y\z via cygpath -w in 5c1a74e definitely got past the initial pwsh iwr failure fwiw.

but based on https://devblogs.microsoft.com/commandline/tar-and-curl-come-to-windows/ it seems windows does have native curl since windows 10 (2018-present - not sure if support needs to go back further than that?) - it works as expected at least in CI, so I moved to that as a unification.

with the broken downloads fixed, we're now onto a shasum failure:

sha256sum: \'science-fat-windows-x86_64.exe\'$\'\\r\\r\': No such file or directory

which seems to indicate some trailing carriage returns that may be messing things up:

om:tmp kw$ cat -e science-fat-windows-x86_64.exe.sha256 
010cfa5ddf8d50482dada3624c044d74dc8e699d7b218d3d8d74a89204a4029c *science-fat-windows-x86_64.exe^M^M$

(with shasum checking removed, the tests pass E2E on the windows shards)

@kwlzn
Copy link
Collaborator Author

kwlzn commented Sep 7, 2024

sanitizing the downloaded sha256 file with tr on windows seems to have done the trick and all tests now pass.

so, I think this is good to go - assuming you're ok with the switch to curl. there appear to be other failures from test_exe.py on the windows shard tho. is that expected?

Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

The remaining wonkiness was just self hosted runner hygiene. I've fixed that up with a custom runner job start / stop script and I'll follow up with a few CI workflow tweaks as well towards that end.

cd "${workdir}"

if [[ "${OS}" == "windows" ]]; then
# N.B. Windows sha256sum is sensitive to trailing \r in files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, my bug:

checksum_file.write_text(f"{digest.hexdigest()} *{dst_binary_name}{os.linesep}")

2 bugs really, using \r\n when science is run on Windows, and using the host OS linesep for a shasum file for a potentially foreign scie. Afaict after some brief research, shasum files are not a thing in Windows; so I can just always use a \n linesep.

Your fix needs to remain to handle old releases. but I'll fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #92

@jsirois
Copy link
Contributor

jsirois commented Sep 7, 2024

Thanks for wading through this Kris.

so, I think this is good to go - assuming you're ok with the switch to curl.

Absolutely - simpler. I was forced to pwsh 2 years ago when getting scie-jump green in CI. I'm not sure what is different this much time passed.

@jsirois jsirois merged commit 351ff9d into a-scie:main Sep 7, 2024
7 checks passed
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.

2 participants