Skip to content

Fix packages incompatibility with Apple M1 by defaulting to linux/amd64 containers #1080

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

Merged
merged 6 commits into from
Feb 13, 2023

Conversation

minhkhul
Copy link
Contributor

@minhkhul minhkhul commented Feb 9, 2023

Summary

Due to package incompatibility, this PR adjusts the Makefile to build and run affected containers using linux/amd64 by default.

Testing

Local make all and make tests has no error and takes around 50s on my M1 macbook.
Screenshot 2023-02-09 at 12 05 41 PM

@minhkhul minhkhul requested a review from krivard February 9, 2023 17:06
@krivard krivard requested a review from melange396 February 10, 2023 15:08
krivard
krivard previously approved these changes Feb 10, 2023
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

Looks good, and thankyou for keeping the line lengths reasonable 🙏

Verified running on my machine with these changes does not affect running time for make all test.

Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

looks pretty good! two things though:

  • (nit) it helps readability and is a good convention to put a space before each line-continuation backslash.

  • (todo) you should make the --platform linux/amd64 optional so that non-m1 environments arent forced to use it by default -- platform auto-selection seems to work properly for all but the m1 mac environment. you could make an m1 argument to force this behavior (see the pdb argument for reference), or even use the output of the command uname -smp to make that determination for you.

Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍 fulfills request for command-line switch

probably the change to pdb handling was unintentional and can be fixed directly from the suggestion; if it was on purpose can you say more about what you need it to do?

melange396
melange396 previously approved these changes Feb 13, 2023
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

fix the pdb thing, and it looks good!

@minhkhul
Copy link
Contributor Author

minhkhul commented Feb 13, 2023

Improvement with uname use. No longer need optional param.
make all test passes on my M1 system.
Screenshot 2023-02-13 at 12 36 14 PM

@minhkhul minhkhul requested a review from melange396 February 13, 2023 17:32
melange396
melange396 previously approved these changes Feb 13, 2023
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

automatic! awesome!!!
i would put an @echo line inside your ifeq block that prints something like "Apple M1 environment detected, using 'linux/amd64' platform for compatibility", so that its more obvious that the alternative platform is being used. but other than that, this is great!

@minhkhul
Copy link
Contributor Author

Added some code to notify user when M1 system is detected.
Screenshot 2023-02-13 at 2 20 10 PM

melange396
melange396 previously approved these changes Feb 13, 2023
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

excellent work!

@minhkhul minhkhul requested a review from krivard February 13, 2023 19:42
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍 great adjustments!

@krivard krivard merged commit c0634a3 into dev Feb 13, 2023
@krivard krivard deleted the m1 branch February 13, 2023 19:44
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.

3 participants