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

fix #25: Support ARM64 and other platforms #44

Merged
merged 6 commits into from
May 24, 2023
Merged

fix #25: Support ARM64 and other platforms #44

merged 6 commits into from
May 24, 2023

Conversation

adamchalmers
Copy link
Contributor

@adamchalmers adamchalmers commented Sep 27, 2022

I've added unit tests, so hopefully this works :) If you approve this PR I would appreciate cutting a new release afterwards, as there's a number of projects which would like to run this on ARM.

@adamchalmers adamchalmers changed the title feature: Support ARM64 and other platforms (#12) fix #25: Support ARM64 and other platforms Sep 27, 2022
@adamchalmers
Copy link
Contributor Author

Hmm, don't merge this, it doesn't properly detect these platforms yet.

@adamchalmers adamchalmers marked this pull request as draft September 27, 2022 17:16
@adamchalmers adamchalmers marked this pull request as ready for review September 29, 2022 14:04
@adamchalmers
Copy link
Contributor Author

Actually, my test wasn't actually running on those weird platforms, it was using cross-compilation. I think setup-protoc should probably let users override os.arch() and specify their own arch/plat in an optional Github Action input if they choose to. But I'll open a separate PR for that.

@adamchalmers
Copy link
Contributor Author

@MatteoPologruto @per1234 Hi! Could I please get a review on this?

src/installer.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
@per1234 per1234 self-assigned this Jan 28, 2023
@per1234 per1234 added type: enhancement Proposed improvement architecture: arm Specific to ARM host architecture topic: code Related to content of the project itself labels Jan 28, 2023
Copy link
Contributor

@alessio-perugini alessio-perugini left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Do you mind addressing the change requested? We'd really like to merge this 🤓

__tests__/main.test.ts Show resolved Hide resolved
adamchalmers and others added 3 commits May 19, 2023 09:49
@adamchalmers
Copy link
Contributor Author

Of course, I applied all suggestions.

@adamchalmers
Copy link
Contributor Author

I can't get the prettier check to pass, despite running npm format. I assume I need some particular version of prettier installed? Or some particular npm/node version?

@alessio-perugini
Copy link
Contributor

alessio-perugini commented May 19, 2023

I can't get the prettier check to pass, despite running npm format. I assume I need some particular version of prettier installed? Or some particular npm/node version?

Might be related to this:

    ["protoc-3.20.2-osx-x86_64.zip", "darwin", "x64"][
      ("protoc-3.20.2-osx-x86_64.zip", "darwin", "x64")
    ],

should be:

    ["protoc-3.20.2-osx-x86_64.zip", "darwin", "x64"],
    ["protoc-3.20.2-osx-x86_64.zip", "darwin", "x64"],

@adamchalmers
Copy link
Contributor Author

adamchalmers commented May 19, 2023

OK, formatter fixed. BTW in your formatting example

    ["protoc-3.20.2-osx-x86_64.zip", "darwin", "x64"],
    ["protoc-3.20.2-osx-x86_64.zip", "darwin", "x64"],

those two are actually duplicates! So I deduplicated it.

All tests pass now.

Copy link
Contributor

@alessio-perugini alessio-perugini left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@adamchalmers
Copy link
Contributor Author

Nice! I can't merge until @per1234 changes their review status, or an admin merges this for me.

@alessio-perugini
Copy link
Contributor

@adamchalmers we forgot to run npm run build. Can you run it and add the changes in a new commit? 🙏

@adamchalmers
Copy link
Contributor Author

Done, I got one error from npm run build

$ npm run build

> [email protected] build
> tsc

node_modules/keyv/src/index.d.ts:60:32 - error TS2304: Cannot find name 'AsyncGenerator'.

60  iterator(namespace?: string): AsyncGenerator<any, void, any>;
                                  ~~~~~~~~~~~~~~


Found 1 error.

@alessio-perugini
Copy link
Contributor

alessio-perugini commented May 23, 2023

Done, I got one error from npm run build

$ npm run build

> [email protected] build
> tsc

node_modules/keyv/src/index.d.ts:60:32 - error TS2304: Cannot find name 'AsyncGenerator'.

60  iterator(namespace?: string): AsyncGenerator<any, void, any>;
                                  ~~~~~~~~~~~~~~


Found 1 error.

It was something related to typescript version. If you rebase this on master and run again npm run build we should fix the issue. We're almost there 💪

EDIT: Actually not needed as we're switching to ncc building. Thank you so much for your contribution! 🥇

@alessio-perugini alessio-perugini dismissed per1234’s stale review May 24, 2023 14:54

Requests for changes correctly submitted. 💪

@alessio-perugini alessio-perugini merged commit 51cd93c into arduino:master May 24, 2023
@adamchalmers
Copy link
Contributor Author

Awesome, thanks for shepherding this through, Alessio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture: arm Specific to ARM host architecture topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants