Default to runner architecture #376

cap10morgan · 2022-08-29T16:43:47Z

Description:
This is the start of defaulting the architecture param to the runner's architecture. Defaulting the architecture param to the return value of os.arch() is the easy part. However, that's only half of the necessary work because different java distributions will have different names for the architectures. So, we need a way to map from the os.arch() values to whatever the distribution uses. ~~I have a proposal implementation for that for temurin only in here for discussion. Once we agree on the approach, I'm happy to implement it for the other distributions too.~~ UPDATE: Added to all distributions now.

Related issue:
#375

Check list:

Mark if documentation changes are required. (I'm happy to make the changes once the approach is finalized.)
Mark if tests were added or updated to cover the changes. (Not yet but I'm also happy to do this if needed.)

src/distributions/temurin/installer.ts

cap10morgan · 2022-08-30T15:07:25Z

Should I go ahead and apply the os.arch() -> distribution name mapping to the other distributions and push that here?

panticmilos · 2022-08-31T12:37:21Z

@cap10morgan hi. There is still check dist workflow failing, you ran npm run build I guess? Also, it would be nice to maybe e2e test with this change with the amd64 arch to confirm everything is working properly. Let's not implement this for other distros yet, just to make sure everything is fine. 🙌

cap10morgan · 2022-09-08T14:04:04Z

@panticmilos Anything else you need me to do here?

panticmilos · 2022-09-12T12:56:52Z

@cap10morgan, it's ok to proceed with the other distributions. Do you need my help with anything besides checking the PR?

cap10morgan · 2022-09-13T14:01:47Z

@cap10morgan, it's ok to proceed with the other distributions. Do you need my help with anything besides checking the PR?

OK, will do. I think I'm all set for now, thanks!

They are mostly all the same, and this can still be overridden when needed

cap10morgan · 2022-09-13T14:45:10Z

Added to all distros

cap10morgan · 2022-09-19T14:45:51Z

~~I'm a little confused by the CI failures. All of the tests pass locally for me, but I can't find the failing expectation in my local code. Is the CI running different test code somehow?~~ Just needed a main pull and merge. I'll push a fix shortly.

cap10morgan · 2022-09-19T18:39:37Z

@panticmilos I think this is ready for a new CI run & review.

panticmilos · 2022-09-20T12:59:01Z

hi @cap10morgan, yup all seems correct. Could you add unit tests for distributions other than Temurin? Also, we are missing e2e tests. I can help you with writing these tests as well

cap10morgan · 2022-09-20T14:39:34Z

hi @cap10morgan, yup all seems correct. Could you add unit tests for distributions other than Temurin? Also, we are missing e2e tests. I can help you with writing these tests as well

Yeah I can add other distro tests. Would love to see what exactly you have in mind for the e2e tests.

...and update temurin's to test 2 architectures instead of 1.

cap10morgan · 2022-09-20T16:05:52Z

@panticmilos OK I just pushed more unit tests. If you can point out an example of the kind of e2e tests you have in mind or otherwise clarify what you want to see, I'm happy to work on that too.

panticmilos · 2022-09-21T11:58:18Z

@cap10morgan I have talked with some team members, and the decision is that e2e tests for this case are not necessary.
Since our runners are already working with certain architectures, it would be hard through e2e tests to simulate other architectures, and existing tests are already covering no architecture input.

Following days I am going to finally test your PR with some self-hosted runners, and then we can proceed to merge it :)

Thank you for your work!

cap10morgan · 2022-09-21T14:30:07Z

@panticmilos Sounds good, thanks! 06bcb9d should fix the CI failure.

cap10morgan · 2022-09-23T16:25:49Z

@panticmilos Resolved conflicts w/ main branch. Anything else needed to merge?

panticmilos · 2022-09-26T09:56:30Z

Hi @cap10morgan, we will delay merging for a bit, because today we want to do the release. This feature will be included in the next release after this one :)

dmitry-shibanov · 2022-10-05T08:11:15Z

Hello @cap10morgan. Thank you for your pull request. I think we need to remove default value from action.yml because otherwise it will always default default to x64.

...so that it can default to the runner's architecture.

cap10morgan · 2022-10-05T16:53:18Z

Hello @cap10morgan. Thank you for your pull request. I think we need to remove default value from action.yml because otherwise it will always default default to x64.

Good catch! Done.

action.yml

dmitry-shibanov · 2022-10-05T20:16:08Z

action.yml

    required: false
-    default: 'x64'
+    default: ''


Could you please remove it because by default the gitInput will return an empty string.

Sure, I was just trying to see if this was the source of the CI errors. I guess not.

dmitry-shibanov · 2022-10-05T20:16:14Z

src/distributions/base-installer.ts

@@ -25,7 +26,7 @@ export abstract class JavaBase {
    ({ version: this.version, stable: this.stable } = this.normalizeVersion(
      installerOptions.version
    ));
-    this.architecture = installerOptions.architecture;
+    this.architecture = installerOptions.architecture || os.arch();


I think os.arch() check is better to move to setup-java.ts

That's much harder to test, and would break all of the tests I added for this.

dmitry-shibanov · 2022-10-05T20:17:02Z

Could you please sync with the main branch and run the npm ci && npm run build command.

This reverts commit 2f647ea.

cap10morgan · 2022-10-06T15:20:47Z

Could you please sync with the main branch and run the npm ci && npm run build command.

Done

cap10morgan · 2022-10-10T15:08:32Z

@dmitry-shibanov @panticmilos anything else needed here?

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [actions/setup-java](https://togithub.com/actions/setup-java) | action | minor | `v3.5.1` -> `v3.6.0` | --- ### Release Notes <details> <summary>actions/setup-java</summary> ### [`v3.6.0`](https://togithub.com/actions/setup-java/releases/tag/v3.6.0) [Compare Source](https://togithub.com/actions/setup-java/compare/v3.5.1...v3.6.0) In scope of this release we added [Maven Toolchains Support](https://togithub.com/actions/setup-java/issues/276) and [Maven Toolchains Declaration](https://togithub.com/actions/setup-java/pull/282). Moreover, from this release we use `os.arch` to determine default architecture for runners: [https://github.com/actions/setup-java/pull/376](https://togithub.com/actions/setup-java/pull/376). Besides, we made such changes as: - [Upgrade @actions/cache from 3.0.0 to 3.0.4](https://togithub.com/actions/setup-java/pull/392) so it respects `SEGMENT_DOWNLOAD_TIMEOUT_MINS` - [Support Gradle version catalog](https://togithub.com/actions/setup-java/pull/394) - [Update @actions/core to 1.10.0](https://togithub.com/actions/setup-java/pull/390) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/Yash-Garg/qBittorrent-KT).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

cap10morgan added 2 commits August 29, 2022 10:37

Default architecture param to os.arch()

7388782

Map temurin arch names to node os.arch() names

b090764

cap10morgan requested a review from a team August 29, 2022 16:43

panticmilos reviewed Aug 29, 2022

View reviewed changes

src/distributions/temurin/installer.ts Outdated Show resolved Hide resolved

panticmilos approved these changes Aug 29, 2022

View reviewed changes

marko-zivic-93 approved these changes Aug 30, 2022

View reviewed changes

Simplify osArchToDistributionArch switch statement

7e83eb1

Format my code w/ prettier

148d9af

cap10morgan added 5 commits August 31, 2022 07:12

Commit build artifact

a115f58

Move os.arch() defaulter to JavaBase constructor

cca7653

Refactor arch mapping to distributionArchitecture method

e169777

Add tests for os.arch() default behavior

c22bb80

Commit build artifact

46869be

tuckstarrydell referenced this pull request Sep 8, 2022

Add support for multiple jdks (#368)

d854b6d

cap10morgan added 2 commits September 13, 2022 08:43

Move distributionArchitecture method to base class

f21b8ec

They are mostly all the same, and this can still be overridden when needed

Map OS arch to distro arch for other distros

0b04eaf

cap10morgan added 2 commits September 13, 2022 09:06

Run format

7284eee

Run build

43c9a22

cap10morgan added 2 commits September 19, 2022 08:58

Merge branch 'main' into feature/default-to-runner-architecture

36a2b11

Default architecture to x86 in test

f83416a

Add arch default tests to all distros

c041f7c

...and update temurin's to test 2 architectures instead of 1.

Fix microsoft installer test on non-macOS platforms

06bcb9d

cap10morgan added 2 commits September 23, 2022 10:24

Merge branch 'main' into feature/default-to-runner-architecture

da28779

Run npm format

da9079a

Remove default architecture value

df9446d

...so that it can default to the runner's architecture.

Use blank string for architecture default value

2f647ea

dmitry-shibanov reviewed Oct 5, 2022

View reviewed changes

cap10morgan added 2 commits October 6, 2022 09:15

Revert "Use blank string for architecture default value"

a1a9075

This reverts commit 2f647ea.

Merge branch 'main' into feature/default-to-runner-architecture

2dec309

cap10morgan added 2 commits October 7, 2022 07:45

Use double quotes in architecture description

04e13d1

Merge branch 'main' into feature/default-to-runner-architecture

7f90be4

cap10morgan mentioned this pull request Oct 7, 2022

Default architecture to runner's architecture #375

Closed

dmitry-shibanov approved these changes Oct 10, 2022

View reviewed changes

dmitry-shibanov merged commit 3617c43 into actions:main Oct 10, 2022

dmitry-shibanov mentioned this pull request Dec 4, 2022

Derive architecture from underlying runner machine architecture #418

Closed

yeikel pushed a commit to yeikel/setup-java that referenced this pull request Feb 12, 2023

Default to runner architecture (actions#376)

a0ed1ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to runner architecture #376

Default to runner architecture #376

cap10morgan commented Aug 29, 2022 •

edited

Loading

cap10morgan commented Aug 30, 2022

panticmilos commented Aug 31, 2022

cap10morgan commented Sep 8, 2022

panticmilos commented Sep 12, 2022

cap10morgan commented Sep 13, 2022

cap10morgan commented Sep 13, 2022

cap10morgan commented Sep 19, 2022 •

edited

Loading

cap10morgan commented Sep 19, 2022

panticmilos commented Sep 20, 2022

cap10morgan commented Sep 20, 2022

cap10morgan commented Sep 20, 2022

panticmilos commented Sep 21, 2022

cap10morgan commented Sep 21, 2022

cap10morgan commented Sep 23, 2022

panticmilos commented Sep 26, 2022

dmitry-shibanov commented Oct 5, 2022

cap10morgan commented Oct 5, 2022

dmitry-shibanov Oct 5, 2022

cap10morgan Oct 6, 2022

dmitry-shibanov Oct 5, 2022

cap10morgan Oct 6, 2022

dmitry-shibanov commented Oct 5, 2022

cap10morgan commented Oct 6, 2022

cap10morgan commented Oct 10, 2022

Default to runner architecture #376

Default to runner architecture #376

Conversation

cap10morgan commented Aug 29, 2022 • edited Loading

cap10morgan commented Aug 30, 2022

panticmilos commented Aug 31, 2022

cap10morgan commented Sep 8, 2022

panticmilos commented Sep 12, 2022

cap10morgan commented Sep 13, 2022

cap10morgan commented Sep 13, 2022

cap10morgan commented Sep 19, 2022 • edited Loading

cap10morgan commented Sep 19, 2022

panticmilos commented Sep 20, 2022

cap10morgan commented Sep 20, 2022

cap10morgan commented Sep 20, 2022

panticmilos commented Sep 21, 2022

cap10morgan commented Sep 21, 2022

cap10morgan commented Sep 23, 2022

panticmilos commented Sep 26, 2022

dmitry-shibanov commented Oct 5, 2022

cap10morgan commented Oct 5, 2022

dmitry-shibanov Oct 5, 2022

Choose a reason for hiding this comment

cap10morgan Oct 6, 2022

Choose a reason for hiding this comment

dmitry-shibanov Oct 5, 2022

Choose a reason for hiding this comment

cap10morgan Oct 6, 2022

Choose a reason for hiding this comment

dmitry-shibanov commented Oct 5, 2022

cap10morgan commented Oct 6, 2022

cap10morgan commented Oct 10, 2022

cap10morgan commented Aug 29, 2022 •

edited

Loading

cap10morgan commented Sep 19, 2022 •

edited

Loading