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.

Sign up for GitHub

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

Jump to bottom

Support architecture parameter #591

Closed
wants to merge 2 commits into from
Closed

Support architecture parameter #591

wants to merge 2 commits into from

Conversation

dcvz
Copy link

@dcvz dcvz commented Nov 7, 2023 •
edited
Loading

Changes

  • Provides --architecture parameter for support on running on M1 agents

Related Issues

  • TBD

Checklist

  • Read the contribution guide and accept the
    code of conduct
  • Docs (If new inputs or outputs have been added or changes to behavior that should be documented. Please make a PR
    in the documentation repo)
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023

Cat Gif

@dcvz dcvz marked this pull request as ready for review November 7, 2023 09:30
webbertakken
webbertakken approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@webbertakken webbertakken left a comment

Choose a reason for hiding this comment

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

Nice one!

webbertakken
webbertakken reviewed Nov 7, 2023
View reviewed changes
src/model/platform-setup/setup-mac.ts Outdated
const architectureArgument = [];

// @ts-ignore
const process = require('process');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but I don't think you need to import process and use TS ignore. Node has process as a global.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, alright! I'll try with the change

Copy link
Author

Choose a reason for hiding this comment

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

Changed made, and it works too!

@dcvz
Copy link
Author

dcvz commented Nov 7, 2023 •
edited
Loading

Looks like CI builds are failing because of the license server

@AndrewKahr
Copy link
Member

Hey there, we just merged a fix for the failing license activation. Can you merge main into your branch and link a successful build log from your own branch. Then we can review and merge. Thanks!

@AndrewKahr AndrewKahr mentioned this pull request Nov 15, 2023
Merged
4 tasks
@dcvz
Copy link
Author

dcvz commented Nov 15, 2023

Can take a look at this today! Or is this already part of #596?

@AndrewKahr
Copy link
Member

I've already added it to the other PR and merged it in an effort to get v4 out faster. I will make sure you are credited in the release notes and commit message though for your contribution! Gonna close this PR now since the functionality is now merged to main.

@AndrewKahr AndrewKahr closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webbertakken webbertakken webbertakken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dcvz @AndrewKahr @webbertakken