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

Default to runner architecture #376

Merged

Conversation

cap10morgan
Copy link
Contributor

@cap10morgan cap10morgan commented Aug 29, 2022

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.)

@cap10morgan cap10morgan requested a review from a team August 29, 2022 16:43
@cap10morgan
Copy link
Contributor Author

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

@panticmilos
Copy link
Contributor

@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
Copy link
Contributor Author

@panticmilos Anything else you need me to do here?

@panticmilos
Copy link
Contributor

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

@cap10morgan
Copy link
Contributor Author

@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
Copy link
Contributor Author

Added to all distros

@cap10morgan
Copy link
Contributor Author

cap10morgan commented Sep 19, 2022

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
Copy link
Contributor Author

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

@panticmilos
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@cap10morgan
Copy link
Contributor Author

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

@panticmilos
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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 Outdated Show resolved Hide resolved
action.yml Outdated
required: false
default: 'x64'
default: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@dmitry-shibanov
Copy link
Contributor

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

@cap10morgan
Copy link
Contributor Author

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

Done

@cap10morgan
Copy link
Contributor Author

@dmitry-shibanov @panticmilos anything else needed here?

@dmitry-shibanov dmitry-shibanov merged commit 3617c43 into actions:main Oct 10, 2022
Yash-Garg referenced this pull request in Yash-Garg/qBittorrent-Manager Oct 18, 2022
[![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 @&#8203;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 @&#8203;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.

---

- [ ] <!-- rebase-check -->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).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMzguNCIsInVwZGF0ZWRJblZlciI6IjMyLjIzOC40In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
yeikel pushed a commit to yeikel/setup-java that referenced this pull request Feb 12, 2023
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.

4 participants