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

feat: "zero" install now lives up to its name #1219

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

quintesse
Copy link
Contributor

We're not doing a "hidden install" anymore, but (inefficiently)
downloading Jbang to a temporary location and cleaning it up after
each invocation.

Fixes #1218

@quintesse quintesse requested a review from maxandersen January 26, 2022 14:02
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #1219 (aa93695) into main (d683e0a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1219   +/-   ##
=========================================
  Coverage     58.82%   58.82%           
  Complexity     1057     1057           
=========================================
  Files            86       86           
  Lines          5661     5661           
  Branches        954      954           
=========================================
  Hits           3330     3330           
  Misses         1839     1839           
  Partials        492      492           
Flag Coverage Δ
Linux 57.67% <ø> (ø)
Windows 57.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3adca1b...aa93695. Read the comment docs.

@quintesse
Copy link
Contributor Author

quintesse commented Jan 26, 2022

WDYT about this @maxandersen . I feel this is a better way of handling the "zero install" issue.
I realized that the current way wasn't working as expected, for example with the GUI dialog popup PR I created.
For anyone running an older Jbang or for anyone who previously had run a curl | bash command the popup would not show unless they would first update their local Jbang to the latest version. That just felt really wrong.

So in this new version the startup scripts only run if they find a local jbang.jar (either in the same folder of in a .jbang subfolder). In all other cases the zero-install kicks in and will download the latest Jbang to a temp folder, run jbang and then clean up afterwards. So the scripts will not look anymore to see if you have Jbang installed and run that instead (which is what they do now).

Edit: the cleanup will not touch $HOME/.jbang, so it's not 100% zero-install of course, but I feel that would be going too far :-)
So once a JDK is downloaded it will stay downloaded. Same for any scripts that get compiled, they stay in the cahce.

@maxandersen
Copy link
Collaborator

so just thinking it through - it makes sense; but side-effect is that doing curl-jbang twice now will "cost" more than before, right? not necessarily bad - but if the case it gets connected to how we currently recommend installing quarkus cli where we call curl twice - one for setup trust, other for a run...which is verbose but fast.

thus related to trust workflow.

@quintesse
Copy link
Contributor Author

It's indeed slightly slower to do two curls now, but Jbang isn't that big luckily.
Also, the moment you must do two curl/jbang commands isn't it much better to just install jbang?
And thirdly, with the GUI popup we don't need to do two commands anymore (when not running remotely/in a container at least).

We're not doing a "hidden install" anymore, but (inefficiently)
downloading Jbang to a temporary location and cleaning it up after
each invocation.

Fixes jbangdev#1218
@maxandersen
Copy link
Collaborator

lets not merge this before we still have a working one-liner "app install" option that works, i.e.:

curl -Ls https://sh.jbang.dev | bash -s - app install quarkus@quarkusio

@quintesse
Copy link
Contributor Author

lets not merge this before we still have a working one-liner "app install" option that works

Turns out that this worked all along, the app install already performs a setup if needed.

But while testing this I needed a clean environment so I started a container and realized that in those kind of situations it won't work anyway because a) we're redirecting stdin and b) there's no way to show a GUI popup.

And then I thought: what if we read directly from /dev/tty? It won't work on Windows (I tried using con, con: and conIn$, but none of these seemed to work) but that's fine, this is just so it will work in containers.

Therefore I created this PR: #1312 . We can merge this PR when that one is merged.

@maxandersen
Copy link
Collaborator

merging this so can do a release with this in effect and test over the weekend :)

@maxandersen maxandersen merged commit ea20eff into jbangdev:main Mar 18, 2022
maxandersen added a commit that referenced this pull request Mar 19, 2022
@maxandersen
Copy link
Collaborator

had to revert this as "curl -Ls https://sh.jbang.dev | bash -s - app install --fresh --force quarkus@quarkusio" stopped working.

@quintesse quintesse deleted the zero_install branch March 22, 2022 11:08
quintesse added a commit to quintesse/jbang that referenced this pull request Mar 22, 2022
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.

"zero install" should live up to its name
2 participants