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

bug: add aes support for darwin/arm64 #5

Closed
wants to merge 1 commit into from

Conversation

ainghazal
Copy link
Contributor

x/sys/cpu lacks support for darwin/arm64, so here I am porting over the hardcoded implementation from internal/cpu.

For reference:

@@ -1,4 +1,4 @@
//go:build !android || !arm64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a bug, wasn't it?

@@ -1,4 +1,4 @@
//go:build !android || !arm64
//go:build arm64 && !android && !darwin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure about the original intention, but I think this is the right thing to do here...

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 we should discuss this in our 1:1 today. It may also be that a better pattern would be to have files for each combination of GOOS and GOARCH we support rather than doing smart things with conditionals (as you mentioned, we may already had some issues with conditionals, hence it seems it's better to be a little more humble here and avoid too many conditionals).

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this topic in our 1:1 and @ainghazal is going to write a small internal document to evaluate our options here. We'll explore possibilities such as vendoring internal/cpu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once we have explored options, we'll try to figure out the cheapest and most convenient for us way to fix the bug and ensure that we're in a place where darwin/arm64 is WAI.

bassosimone added a commit that referenced this pull request Aug 18, 2022
This diff ensures we hardcode the capabilities of darwin/arm64.

While there, strive to make the packages naming slightly less confusing.

Reference issue: ooni/probe#2122.

Surpersedes (and draws from) #5.

Co-authored-by: ain ghazal <ainghazal42@gmail.com>
@bassosimone
Copy link
Contributor

Thanks! I have build upon your patch here at #7.

bassosimone added a commit that referenced this pull request Aug 18, 2022
This diff ensures we hardcode the capabilities of darwin/arm64.

While there, strive to make the packages naming slightly less confusing.

Reference issue: ooni/probe#2122.

Surpersedes (and draws from) #5.

Co-authored-by: ain ghazal ainghazal42@gmail.com

Co-authored-by: ain ghazal <ainghazal42@gmail.com>
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.

oocrypto: AES not used on darwin/arm64
2 participants