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 binary artifacts for Mac #21

Merged
merged 6 commits into from
Dec 28, 2024
Merged

Fix binary artifacts for Mac #21

merged 6 commits into from
Dec 28, 2024

Conversation

lispyclouds
Copy link
Member

@lispyclouds lispyclouds commented Dec 28, 2024

Addresses #19

@@ -135,7 +134,7 @@ jobs:
name: Build binary
command: |
source /Users/$(whoami)/.bashrc
GOARCH=arm64 go build -tags "fts5" -o pod-babashka-go-sqlite3 main.go
go build -tags "fts5" -o pod-babashka-go-sqlite3 main.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe be explicit about the environment here and keep GOARCH?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as the other comment, thought to keep the var set only where its needed and only vary the instance types.

@@ -50,10 +50,6 @@ jobs:
command: curl -L https://git.io/vQhTU | bash
- run:
name: Build binary
environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I should add the CGO_ENABLED one, but the GOARCH and GOOS felt like another point of change apart from the instance type its running on. Felt better to remove since we have dedicated instances; would be better to keep if we cross compiled for all in a single instance.

@borkdude
Copy link
Contributor

@lispyclouds I made some comments but maybe you can't see them so I'm posting it as screenshot here as well.

Screenshot 2024-12-28 at 22 35 41 Screenshot 2024-12-28 at 22 35 59

It seems that the only changes needed are to the test build script + test? E.g. why is CGO removed from the linux aarch build?

@lispyclouds
Copy link
Member Author

Answered them, adding the CGO back but want to know of what you think of my reasons for removing the others.

@borkdude
Copy link
Contributor

@lispyclouds I think we should only change something if we are solving a problem. Just changing the test binary name solves the problem so I'd only change that.

@borkdude
Copy link
Contributor

Why are we even building a separate test binary?

@lispyclouds
Copy link
Member Author

lispyclouds commented Dec 28, 2024

IIRC it was to aid in local testing, to not build separately too. The commit was from you: f7f8ab2

@lispyclouds
Copy link
Member Author

only the file changes are there now. we could use the binary built in the prev step for the tests but for arm macs, its going to run the amd binary and not sure if rosetta is setup? or if thats a comprehensive test or not with CGO etc

@borkdude
Copy link
Contributor

Installing rosetta is very simple:

      - run:
          name: Install Rosetta
          command: |
            sudo /usr/sbin/softwareupdate --install-rosetta --agree-to-license

Since rosetta wasn't a problem before, why was a separate test binary used before? I don't get the reason for that still.

@lispyclouds
Copy link
Member Author

Yeah I think we can avoid the test build but like I said before, I think you added it in the shape you had it in your local test setup in f7f8ab2
Having the script like this allows for a faster test cycle without needing to do a go build before testing for every code change

@borkdude
Copy link
Contributor

Alright. Thanks for hunting the bug down! I'll just merge this and make a release tomorrow.

@borkdude borkdude merged commit c8a9219 into main Dec 28, 2024
3 checks passed
@borkdude borkdude deleted the mac-fixes branch December 28, 2024 22:23
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.

2 participants