-
Notifications
You must be signed in to change notification settings - Fork 831
Support 32-bit architecture #57
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
Conversation
6cd3630 to
8b062ac
Compare
|
|
||
| let archExtension = ''; | ||
| if (arch === 'x86') { | ||
| archExtension = 'i686'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this extension come from? Is it not x86?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i686 is the latest iteration of the IA-32 (commonly known as x86). From today's perspective, it's a synonym of x86 (although it is technically not). I didn't choose this extension, I just looked at https://cdn.azul.com/zulu/bin/ and saw that's what they use.
| } else if (arch === 'x64') { | ||
| archExtension = 'x64'; | ||
| } else { | ||
| throw new Error(`architecture "${arch}" is not int [x86 | x64]`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should validate that the architecture input is valid when we retrieve the input in https://github.com/actions/setup-java/blob/master/src/setup-java.ts#L12
Then this whole block can just be:
const archExtension = arch === 'x86' ? 'i686' : 'x64';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I'm not very familiar with the GitHub Actions core library (and JS in general) so bear with me. I'll make the change soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! Let me know if you need any help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "not in" instead of "not int".
|
Hi folks. Any progress updates on this PR? |
|
@konradpabjan This PR can now be closed. |
|
Replaced with #95 |
Fix #55
I'm using this code in this GitHub Actions pipeline: https://github.com/dnp3/opendnp3/actions
I only tried it on Windows. Perhaps some integration tests could be added to verify that the proper version is fetched. I'm not sure to understand how your tests are run, so I didn't do it.