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

fix: making gl thing to compile and work on apple silicon #1922

Merged
merged 3 commits into from
Feb 20, 2021
Merged

fix: making gl thing to compile and work on apple silicon #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44d2d4a
fix: making gl thing to compile and work on apple silicon
sirkon Feb 7, 2021
5648e2a
fix: some boolean algebra applied
sirkon Feb 8, 2021
f93d4e0
fix: darwin is pure desktop with gl core to use
sirkon Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/driver/glfw/glfw_core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// +build !gles,!arm,!arm64
// +build !gles,!arm,!arm64 darwin

package glfw

Expand Down
1 change: 1 addition & 0 deletions internal/driver/glfw/glfw_es.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// +build gles arm arm64
// +build !darwin

package glfw

Expand Down
2 changes: 1 addition & 1 deletion internal/painter/gl/gl_core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// +build !gles,!arm,!arm64,!android,!ios,!mobile
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is not quite right.
If I have a linux arm64 this file should not be activated, but with that removed it will

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, know this. Some options were just off. Rewrite all things with the following ideas in mind:

  1. *_core.go files all getting x OR (arm64 AND darwin), this means they are to be used with MacOS on Apple M1.
  2. *_es.go files all getting `x AND (!arm64 AND darwin OR arm64 !darwin). This means they are excluded when compiling for MacOS on Apple M1.

Where x represents the existing set conditions.

BTW, !arm64 AND darwin OR arm64 !darwin == !(arms64 AND darwin), this is a rule of boolean algebra.
In fact I could simplify the negation in _es files for Apple M1 if keeping existing conditions in mind, but decided to stay it this way to keep things cleaner. You will be able to put another pairs if needed in inclusive conditions and append lines with exclusive conditions with just copy-pasting and minor editing.

Copy link
Member

Choose a reason for hiding this comment

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

I think this has overcomplicated it now, sorry. the ES stuff should never run on darwin, and the core always should (as iOS is a different flag). All of the others were OK as far as I could see, just this one not.
Re-adding the "!arm64" to this line should and adding "arm64,darwin" at the end should have done it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the new change for this file is correct - but all of the other updates were not needed.
I'm not certain that your summary of boolean algebra is correct - you seem to have described exclusive or, which is not !(x and y).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which is not !(x and y)

shit, indeed. Turned all conditions into x AND !darwin for _es and x OR darwin for _core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind though, this will work for Go > 1.16 only.

Copy link
Member

Choose a reason for hiding this comment

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

I cannot see which elements of this change are only supporting Go 1.16?
We support back to 1.12, we will upgrade to 1.14 or 1.15 once Debian releases testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GOOS=darwin for iOS prior Go 1.16. It is GOOS=ios since 1.16.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we embed the gomobile project for building ios and that sets the tag.
Does that mean that there is no problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this indeed

// +build !gles,!arm,!arm64,!android,!ios,!mobile darwin

package gl

Expand Down
1 change: 1 addition & 0 deletions internal/painter/gl/gl_es.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// +build gles arm arm64
Copy link
Contributor

Choose a reason for hiding this comment

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

the previous change in this file was good.
it is compiled on M1 with previous changes.

// +build !android,!ios,!mobile
// +build !darwin

package gl

Expand Down