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

add build & release #1

Merged
merged 5 commits into from
May 22, 2023
Merged

add build & release #1

merged 5 commits into from
May 22, 2023

Conversation

pcunning
Copy link
Contributor

@pcunning pcunning commented Mar 2, 2023

This will build commits to main for RPi, Mac (x86 & ARM) and Windows (386 & ARM) then set is as a release with artifacts. There are probably better ways to do multiple platform builds with matrix builds but this works.

The release step if failing because when running in forks actions run in a read-only context (for security because forks are weird).

Copy link
Owner

@flwyd flwyd left a comment

Choose a reason for hiding this comment

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

This is great, thank you for contributing!

I've got a bunch of comments on specifics, some of which are me trying to figure out the state of computing platforms that I don't use. If I'm off-base on any of these, please clue me in.

.github/workflows/go-build.yml Outdated Show resolved Hide resolved
.github/workflows/go-build.yml Outdated Show resolved Hide resolved
.github/workflows/go-build.yml Outdated Show resolved Hide resolved
.github/workflows/go-build.yml Outdated Show resolved Hide resolved
.github/workflows/go-build.yml Outdated Show resolved Hide resolved
@flwyd
Copy link
Owner

flwyd commented May 14, 2023

Hi @pcunning, any chance you could respond to my comments in the review?

@pcunning pcunning changed the title build go for 5 platforms add build & release May 17, 2023
@pcunning
Copy link
Contributor Author

Sorry to disappear on you. This should look better. You can see the built assets here https://github.com/pcunning/adif-multitool/actions. The release fails because it is on a fork.

I tested the following and both worked:

  • linux-arm on a RPi 4
  • darwin-amd64 on a Intel Mac.

Notes on platforms:

@flwyd flwyd merged commit a098b40 into flwyd:main May 22, 2023
@flwyd
Copy link
Owner

flwyd commented May 22, 2023

Looks great, thanks for the contribution!

@pcunning
Copy link
Contributor Author

Unfortunately it looks like the file matching failed.

https://github.com/flwyd/adif-multitool/actions/runs/5041719540/jobs/9041683400#step:3:37

@flwyd
Copy link
Owner

flwyd commented May 23, 2023

I turned on debug logs, but it doesn't say what $PWD is for this workflow. However, since it says Artifact adif-multitool-darwin-amd64 was downloaded to /home/runner/work/adif-multitool/adif-multitool/adif-multitool-darwin-amd64, I wonder if it's running from /home/runner/work/adif-multitool and all the artifacts are down in a second adif-multitool directory.

@flwyd
Copy link
Owner

flwyd commented May 23, 2023

I changed the artifact name structure to $GOOS-$GOARCH/adifmt subdirectories aren't allowed, so switched over to adifmt-$GOOS-$GOARCH. I'm now passing adif-multitool/** as the files for the release action rather than adif-multitool-* to get the subdirectory, but that also failed: https://github.com/flwyd/adif-multitool/actions/runs/5053932168/jobs/9068318723

The winning move was setting name: adifmt-builds for both the download-artifact and upload-artifact actions, which saved everything to adifmt-* in PWD.

I also noticed this isn't putting .exe on the end of Windows builds, which might be a problem. Also, when I downloaded a MacOS executable from https://github.com/flwyd/adif-multitool/releases/tag/latest it doesn't have the executable set (which is a reasonable security default). I wonder if it would make sense to package them as a zip/tarball so file metadata can be preserved. This would also allow the archive file name to have the OS and architecture, but the executable inside to just be named adifmt or adifmt.exe. This archive could also have a short platform-specific README explaining how to install the file.

It also looks like the automated release action might break next week: marvinpinto/actions#544
Will need to monitor that situation.

@flwyd
Copy link
Owner

flwyd commented May 23, 2023

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