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 check-latest flag #141

Merged

Conversation

dmitry-shibanov
Copy link
Contributor

Description:
In scope of this pull request we add support for check-latest flag.
Related issue: https://github.com/actions/virtual-environments-internal/issues/1942

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

Copy link
Collaborator

@konradpabjan konradpabjan left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@@ -24,20 +25,29 @@ export abstract class JavaBase {
));
this.architecture = installerOptions.architecture;
this.packageType = installerOptions.packageType;
this.checkLatest = installerOptions.checkLatest ?? false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't need the ?? false since getBooleanInput defaults to it if there is no value

@giltene
Copy link
Contributor

giltene commented Mar 19, 2021

The default should be true, not false. If someone wants to avoid checking for latest update, that may be ok (although I don't see why anyone would choose to do that, ever). But defaulting to that behavior is problematic for many common use cases of setup-java.

@dmitry-shibanov
Copy link
Contributor Author

Hello. Thank you for your participation and help with improving setup-java. We prefer to keep check-latest flag as false for default cases, because we want to reduce task duration that is why we prefer to use our image cache as much as possible. Related comment.

@giltene
Copy link
Contributor

giltene commented Mar 22, 2021

Is there some other place or forum where these discussions and decisions are being made? Who is the team making the decisions? Can I join that team?
Given the dramatic impact proposed with this change, and in a few related changes that seem to be taking shape, a more transparent process that would allow significantly more user input prior to making final choices would be beneficial.

@konradpabjan
Copy link
Collaborator

konradpabjan commented Mar 22, 2021

@giltene

The default should be true, not false. If someone wants to avoid checking for latest update, that may be ok (although I don't see why anyone would choose to do that, ever)

It's important to keep in mind the use-case we're trying to optimize here for. Since late last year our hosted-images come with a few versions of Java from adopt. There are no versions of Java from Zulu that are cached for a while now (there used to be versions of Zulu that were cached but we switched over): https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#java

How often are these versions of adopt that are cached updated? Since October of last year, they've been updated 3 times and on average it takes ~4 days for our images to get updated with the latest versions and roll-out to everyone. Sometimes this update process takes more, sometimes it takes less.

If a cached version of adopt is used vs downloaded from scratch, the time benefit is ~0s vs ~20s. If check-latest is set to true during these update times then the setup-java step would all of a sudden take a longer period of time (not ideal experience for users). Also the time benefit of this cannot be understated from a compute perspective for hosted runners that Github manages. 20 seconds doesn't sound like much, but if you factor in the millions of runs, and VMs in hosted that we have to spin up, re-image etc. 20 seconds each on a common setup action is a pretty big deal. By setting check-latest to true there would be no benefit for us. Also for the vast majority of users I would argue that waiting the 4 days or so for the versions of java to be updated is not all that important and they can just wait until the cached images get updated.

So for hosted runners that use setup-java, the benefit for check-latest being false is pretty clear to me. If you're using another vendor then it makes no difference if check-latest is false or true on hosted since the distribution will have to be downloaded from scratch every single time since nothing else is cached.

For self-hosted runners there is a slightly different use case and I would argue that it makes sense to have check-latest set to true if you want the latest and greatest. However, we don't have a flag or env variable or anything that denotes something like isSelfHosted. The discussion has come up in a few other first party actions if we should have different behaviors for actions that are on hosted vs self-hosted and the answer that came up was no. This could be problematic for matrix builds for example that have some self-hosted runners and some hosted. So for consistency we just said no difference in behavior vs self-hosted and hosted across the board.

That being said, check-latest being false for self-hosted isn't all that bad and I think the pros still outweighs the cons (some might say this doesn't, and that's fair, but for us optimizing for hosted is orders of magnitude more important). Why?

  • Currently setup-java V1 behaves as if check-latest is set to false. No breaking changes with V2 here. This just gives users more flexibility
  • I like to see it almost as an auto-update flag. Having your builds start failing all of a sudden because a java distribution auto-updated could prove problematic, so pinning to a cached version always is a more "stable" CI environment. If someone does want the latest version then that is something they should opt-in to.
  • In setup-node there is also a check-latest option that has been around for a while, and it's worked great. It's set to false so for consistency and we with other first-party actions we should do the same: https://github.com/actions/setup-node/blob/main/action.yml#L12-L14

Is there some other place or forum where these discussions and decisions are being made?

The core specs for setup-java V2 are documented in this PR and for the most part that what we're following and implementing: #97
https://github.com/actions/setup-java/blob/main/docs/adrs/0000-v2-setup-java.md

This check-latest flag was something we figured, why not? Let's add this option in for v2 since it has valid use cases and there has been an ask for this feature for a while, we can squeeze it in and it will be a nice "cherry-on-top".

Given the dramatic impact proposed with this change

For hosted this would clearly benefit us while for self-hosted it behaves just like V1 so IMO characterizing this change as dramatic is a bit of an overstatement

@ibauersachs
Copy link

@konradpabjan I'm confused. Since when has (or had) the pre-installed Java version of the runner anything to do with this action? Does (or did) the action take advantage of the pre-installed Java version, i.e. something like if (version == 11 && distribution == 'adopt' && arch == 'x64') return;?
If the pre-installed version has nothing to do with what setup-java does (which I sincerely hope), then the entire reply is meaningless.

@giltene
Copy link
Contributor

giltene commented Mar 22, 2021

This seems like a strange,, distro-specific, image-dependent, and always-stale (by default) way to address the 20 second startup-due-to-no-caching problem. And it does so by overriding [by default] the specified version spec, and providing a stale (multi-months-old) version even when the requested version spec says to use the latest update available. Setup-java has one main role: to set up the java version spelled out in SemVer format in the java-version property. It should not have an "and I really mean it" flag that defaults to "off".

Instead, the actual core concern of a 20 second start can be addressed just as well (or to within fraction of a second of) by addressing #84 directly. I would be happy to do the work to address that one, and to commit to build it such that it will support caching for all distros to avoid distro favoritism arguments.

Since this is definitely a breaking change being proposed in v2 preview, and a dramatic modification to the common use cases and how OpenJDK updates will flow through to testing. The opportunity to "fix it" with a followup breaking change (that will restore semantics to v1 behavior but provide caching benefits to all distros) will not come up again until v3 [if it is allowed to go into v2 in the current, defaul-is-false proposed behavior]. I therefore strongly object to adding this new property and choosing it's initial default behavior to contradict current v1 behavior. The number of [detectable only during certain days in a year] errors and lapses this will likely cause in the existing ~70K workflows that use the existing semantics will be huge if the default is not set yo be compatible with current behavior....

@konradpabjan
Copy link
Collaborator

Since when has (or had) the pre-installed Java version of the runner anything to do with this action? Does (or did) the action take advantage of the pre-installed Java version, i.e. something like if (version == 11 && distribution == 'adopt' && arch == 'x64') return;?

Currently it doesn't take advantage of any cached distributions because our images only cache some adopt distributions while V1 only is structured to pull Zulu distributions: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#java

There was a time though when our hosted images cached Zulu distributions but we've switched over, see: actions/runner-images#1057
Even when Zulu distributions were cached, our tool/directory structure wasn't set up to easily consume and switch between different cached versions so we didn't fully take advantage of this. Part of V2 involves actually using cached distributions on our images when possible to speed up setup and the work has already been done for that.

If the pre-installed version has nothing to do with what setup-java does (which I sincerely hope)

They do. I wholeheartedly disagree with notion that setup actions should avoid anything with pre-installed versions. There are overwhelming benefits. A good example is the work we did with setup-python v2 that we're now trying to bring over to setup-java. There are 6 versions of CPython that are cached on a hosted Image and 3 versions of PyPy (you can think of these as two different "distributions"): https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#python

There are WAY more versions available than the 9 that are cached on each image and you can download those on the fly, the disadvantage though is that downloading and setting up a version dynamically can take a long time (over a minute on Windows for example!). The setup-python action has thus been optimized to use cached distributions when possible for the most possible versions. The cached versions have a slightly slower update cadence but for the majority of users this is fine and there are ways to get the latest and greatest if you're insistent on using the most up-to-date releases. You can read up how setup-java handles using pre-cached versions here: https://github.com/actions/setup-python#available-versions-of-python

With setup-java you can imagine someone in V2 using version == 11 && distribution == 'adopt and our images have 11.0.10 cached. Now let's assume 11.0.20 comes out. Now, realistically it will take some time to update and test our images with 11.0.20 and we update it eventually, but before that the actions shouldn't differ to download 11.0.20 and effectively disregard whatever is in the cache. When possible we want to use it to make setup as fast as possible. From some of our other actions, users are very vocal about prioritizing faster runs as their biggest requests. If someone really wants 11.0.20 before it's on our image then they can specify check-latest as true with the option in this issue or just specify version == 11.0.20


Instead, the actual core concern of a 20 second start can be addressed just as well (or to within fraction of a second of) by addressing #84 directly. I would be happy to do the work to address that one, and to commit to build it such that it will support caching for all distros to avoid distro favoritism arguments.

Solutions such as those outlined in #84 aren't optimal because actions/cache still involve a step that downloads from somewhere (although this in theory should be a little bit faster). The absolute fastest solution is to cache the most used distributions on the images directly and use those when possible reducing the most common setup time to ~0s. The 20 second improvement was just one example that I mentioned. With Python there are 9 cached versions while with Java we have 2 that we aren't even using now. The more we add the benefits will increase significantly. We can't cache everything because there is a limited amount of storage on our runners and there is a host of other software that users want cached. So fundamentally the problem we're trying to solve is optimizing for the most common use cases across the board

It should not have an "and I really mean it" flag that defaults to "off".

The most common asks that we've overwhelmingly seen are people wanting faster builds which is why the default is off. If you're an open source repo or organization that really needs the latest updates of Java as soon as possible always, then you an easily opt-in, but most users that we've seen don't fall into that bucket. There will be documentation added for this like in setup-python that should clear any confusion about how we determine what is used.

Since this is definitely a breaking change being proposed in v2 preview, and a dramatic modification to the common use cases and how OpenJDK updates will flow through to testing

I would like to reiterate that characterizing this change as dramatic is a bit of an overstatement. It's a new release, so it's acceptable to make any breaking changes. If you follow our migration guide and you're currently using v1 with zulu, this change has no impact on hosted runners because there are no zulu distributions that are cached. So checks-latest being either true or false makes no difference whatsoever since a fresh VM will always require a download to take place. Our other actions like setup-node also have a checks-latest flag that defaults to false so this is a pattern we are implementing across the board as it best help optimize the action for the majority of users: https://github.com/actions/setup-node/blob/5c355be17065acf11598c7a9bb47112fbcf2bbdc/action.yml#L12-L14

@ibauersachs
Copy link

Since when has (or had) the pre-installed Java version of the runner anything to do with this action? Does (or did) the action take advantage of the pre-installed Java version, i.e. something like if (version == 11 && distribution == 'adopt' && arch == 'x64') return;?

Currently it doesn't take advantage of any cached distributions because our images only cache some adopt distributions while V1 only is structured to pull Zulu distributions: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#java

That just states that two random Java versions are installed in the image and that I could run java or javac without installing anything. If I wanted to do that, I wouldn't use setup-java.

There was a time though when our hosted images cached Zulu distributions but we've switched over, see: actions/virtual-environments#1057
Even when Zulu distributions were cached, our tool/directory structure wasn't set up to easily consume and switch between different cached versions so we didn't fully take advantage of this. Part of V2 involves actually using cached distributions on our images when possible to speed up setup and the work has already been done for that.

A cache is something you populate and reuse as long as it isn't outdated, based on a constraint (time, memory, ...). A pre-installed tool on a VM image is NOT a cache. At best you can pre-populate a cache in a VM image.

If the pre-installed version has nothing to do with what setup-java does (which I sincerely hope)

They do. I wholeheartedly disagree with notion that setup actions should avoid anything with pre-installed versions. There are overwhelming benefits. A good example is the work we did with setup-python v2 that we're now trying to bring over to setup-java. There are 6 versions of CPython that are cached on a hosted Image and 3 versions of PyPy (you can think of these as two different "distributions"): https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#python

Python is broken. It needs to be installed and cannot simply be downloaded into a directory and added to $PATH. Please don't use such a broken toolchain as an example.
And again: cache != preinstalled. But it seems that Actions abuses the word cache und thus causes my confusion.

There are WAY more versions available than the 9 that are cached on each image and you can download those on the fly, the disadvantage though is that downloading and setting up a version dynamically can take a long time (over a minute on Windows for example!). The setup-python action has thus been optimized to use cached distributions when possible for the most possible versions. The cached versions have a slightly slower update cadence but for the majority of users this is fine and there are ways to get the latest and greatest if you're insistent on using the most up-to-date releases. You can read up how setup-java handles using pre-cached versions here: https://github.com/actions/setup-python#available-versions-of-python

So is python-versions an indication that there is soon going to be GitHub OpenJDK for Actions? If not, then the point is again that Python is broken.

With setup-java you can imagine someone in V2 using version == 11 && distribution == 'adopt and our images have 11.0.10 cached preinstalled.

Now let's assume 11.0.20 comes out. Now, realistically it will take some time to update and test our images with 11.0.20 and we update it eventually, but before that the actions shouldn't differ to download 11.0.20 and effectively disregard whatever is in the cache. When possible we want to use it to make setup as fast as possible.

Of course it should be fast, but then 11.0.20 needs to be available as soon as api.adoptopenjdk.net gives it back. Days later is too late. Not even apt-get update && apt-get upgrade would be so slow.

From some of our other actions, users are very vocal about prioritizing faster runs as their biggest requests. If someone really wants 11.0.20 before it's on our image then they can specify check-latest as true with the option in this issue or just specify version == 11.0.20

Yes, hurray, I want faster runs too, especially on Macs! But a) not at the cost of having an arbitrary preinstalled version vs. what comes back from api.adoptopenjdk.net. and b) the time for a run is mostly spent in slow builds, probably due to random disk access, not in setup-java. In dnsjava/dnsjava, the JDK setup takes anywhere between 3 to 8s on Ubuntu, and it includes an (unnecessary) download. That is often faster than restoring the Maven cache. On Windows it's 7 to 12s and it's always faster than restoring the Maven cache.

I will definitely always set check-latest: true.

Instead, the actual core concern of a 20 second start can be addressed just as well (or to within fraction of a second of) by addressing #84 directly. I would be happy to do the work to address that one, and to commit to build it such that it will support caching for all distros to avoid distro favoritism arguments.

Solutions such as those outlined in #84 aren't optimal because actions/cache still involve a step that downloads from somewhere (although this in theory should be a little bit faster). The absolute fastest solution is to cache the most used distributions on the images directly and use those when possible reducing the most common setup time to ~0s. The 20 second improvement was just one example that I mentioned. With Python there are 9 cached versions while with Java we have 2 that we aren't even using now. The more we add the benefits will increase significantly. We can't cache everything because there is a limited amount of storage on our runners and there is a host of other software that users want cached. So fundamentally the problem we're trying to solve is optimizing for the most common use cases across the board

And there's again the profound confusion between preinstalled and cache. I don't know what you're basing those 20s on, but with #84 that would come down to probably < 1s. At the benefit of doing what you expect and having smaller, easier maintainable VM images.

It should not have an "and I really mean it" flag that defaults to "off".

The most common asks that we've overwhelmingly seen are people wanting faster builds which is why the default is off. If you're an open source repo or organization that really needs the latest updates of Java as soon as possible always, then you an easily opt-in, but most users that we've seen don't fall into that bucket. There will be documentation added for this like in setup-python that should clear any confusion about how we determine what is used.

Since this is definitely a breaking change being proposed in v2 preview, and a dramatic modification to the common use cases and how OpenJDK updates will flow through to testing

I would like to reiterate that characterizing this change as dramatic is a bit of an overstatement. It's a new release, so it's acceptable to make any breaking changes. If you follow our migration guide and you're currently using v1 with zulu, this change has no impact on hosted runners because there are no zulu distributions that are cached. So checks-latest being either true or false makes no difference whatsoever since a fresh VM will always require a download to take place. Our other actions like setup-node also have a checks-latest flag that defaults to false so this is a pattern we are implementing across the board as it best help optimize the action for the majority of users: https://github.com/actions/setup-node/blob/5c355be17065acf11598c7a9bb47112fbcf2bbdc/action.yml#L12-L14

@konradpabjan
Copy link
Collaborator

That just states that two random Java versions are installed in the image and that I could run java or javac without installing anything. If I wanted to do that, I wouldn't use setup-java

One version is certainly there so that something is in PATH by default, but why then would we have to 2 (we can always add more in the future and we probably will)? Same goes for other software that are pre-installed on our images. Why do we have 4 versions of Ruby and 3 versions of Node, the answer is the to speed up the time it takes users to use these tools and to optimize for the most common use cases.

cache != preinstalled. But it seems that Actions abuses the word cache und thus causes my confusion.

The most basic definition of caching that i could find online is store away in hiding or for future use, and that's exactly what we're doing here. There are different ways of caching lets be clear, but we wouldn't have an NPM module called @actions/tool-cache and an env variable and directory on all our runners called RUNNER_TOOL_CACHE if there wasn't some caching of common tools going on behind the scenes.

So is python-versions an indication that there is soon going to be GitHub OpenJDK for Actions? If not, then the point is again that Python is broken.

Please no 😄 There are enough Java distributions out there and the reason why Python is more complex is because for most platforms you can only download the source code (I'm talking officially from Python.org, you can find a host of ready to install versions but we don't want to use that) and it's up to you to actually compile it (which we do with optimizations)

I will definitely always set check-latest: true

That's the thing, it's an option and you can customize it to your needs and as long as it's documented then there shouldn't be an issue. Regarding a sensible default, that's when things get very opinionated depending on how you use Java and Actions. We're making the deliberate decision here to optimize and favor certain use cases over others by our choice

docs/advanced-usage.md Outdated Show resolved Hide resolved
docs/advanced-usage.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
@maxim-lobanov maxim-lobanov merged commit 022e86d into actions:v2-preview Mar 24, 2021
@giltene
Copy link
Contributor

giltene commented Mar 31, 2021

I'd like to re-iterate that a default value of "false" for check-latest in the proposed v2-preview is a dramatic and unwelcome change in behavior, and one that will be hard to change if v2.0 ends up being released with this surprising default behavior in place.

An option to not actually get the latest version that matches your SemVer version spec may make sense, in which case people who want this new but inconsistent behavior (of semi-randomly picking a version based on what happen to be one the image, and of not updating to the latest versions when they become available) in order to gain some temporary speed benefits for some specific distro versions, can choose to turn the option on by saying "check-latest: false". But since doing that this is a clear counter-to-best-practice behavior from a configuration control point of view, it should be something you choose to do explicitly, and definitely not be the default behavior.

Let's be clear on somatic meaning here: "check-latest: false" is the semantic equivalent of the following statements:
"Actually include the security fixes expected in the specified version": false.
"Actually include the bug fixes expected in the specified version": false.
"Actually include the behavior enhancements expected in the specified version": false.
"Actually include the new bugs introduced in the specified version": false.

Non of those are a good "default" behavior in a CI/CD environment. Especially so in an echo-system where distros update 4+ times a year, with a well published list of 100s of changes and often 10s of security changes.

And yes, "test my repo against the latest released version of Java 11" is a common thing to do, with the intent of finding out if that latest release breaks your stuff. As a practical example, OpenJDK 11.0.9 broke a bunch of stuff that was detected this way, leading to the release of 11.0.9.1.

@giltene
Copy link
Contributor

giltene commented Mar 31, 2021

Is there some other place or forum where these discussions and decisions are being made?

The core specs for setup-java V2 are documented in this PR and for the most part that what we're following and implementing: #97
https://github.com/actions/setup-java/blob/main/docs/adrs/0000-v2-setup-java.md

This check-latest flag was something we figured, why not? Let's add this option in for v2 since it has valid use cases and there has been an ask for this feature for a while, we can squeeze it in and it will be a nice "cherry-on-top".

Since I can't find any mention of adding locally-pre-installed distros or of overriding the provided SemVer version by default
(or even with an option) in the ADR, the only [public] records of discussing this property seem to be in this PR. given the impact of such a change to default behavior, a "we figured why not" certainly needs more review. I've opened the #148 to discuss it.

maxim-lobanov added a commit that referenced this pull request Apr 5, 2021
* actions/setup-java@v2 - Support different distributions (#132)

* Implement support for custom vendors in setup-java

* minor improvements

* minor refactoring

* Add unit tests and e2e tests

* Update documentation for setup-java@v2 release

* minor improvements

* regenerate dist

* fix comments

* resolve comments

* resolve comments

* fix tests

* Update README.md

Co-authored-by: George Adams <george.adams@microsoft.com>

* Apply suggestions from code review

Co-authored-by: Konrad Pabjan <konradpabjan@github.com>

* fix minor nitpicks

* handle 4th digit

* pull latest main

* Update README.md

* rename adoptium to adopt

* rename adoptium to adopt

* rename adoptium to adopt

* Update README.md

* make java-version and distribution required for action

* update readme

* fix tests

* fix e2e tests

Co-authored-by: George Adams <george.adams@microsoft.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>

* Add "overwrite-settings" input parameter (#136)

* add overwrite-settings parameter

* fix e2e tests

* print debug

* fix e2e tests

* add comment

* remove comment

* Add "Contents/Home" postfix on macOS if provider creates it (#139)

* Update e2e-versions.yml

* Update e2e-versions.yml

* implement fix

* Update e2e-versions.yml

* Update installer.ts

* fix filter logic

* Update e2e-versions.yml

* remove extra logic

* Update e2e-versions.yml

* Add check-latest flag (#141)

* add changes for check-latest

* run prerelease script

* resolving comments

* fixing tests

* fix spelling

* improve core.info messages

* run format

* run prerelease

* change version to fix test

* resolve comment for check-latest

* Update README.md

* added hosted tool cache section

* Apply suggestions from code review

Co-authored-by: Maxim Lobanov <v-malob@microsoft.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>

* Avoid "+" sign in Java path in v2-preview (#145)

* try to handle _ versions

* more logs

* more debug

* test 1

* more fixes

* fix typo

* Update e2e-versions.yml

* add unit-tests

* remove debug info from tests

* debug pre-cached versions

* change e2e tests to ubuntu-latest

* update npm licenses

Co-authored-by: George Adams <george.adams@microsoft.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
Co-authored-by: Dmitry Shibanov <dmitry-shibanov@github.com>
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.

None yet

5 participants