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

Update CI release builds to remove goxc #1340

Closed
wants to merge 0 commits into from

Conversation

iofq
Copy link
Contributor

@iofq iofq commented Sep 18, 2024

Fixes #

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@iofq iofq mentioned this pull request Sep 18, 2024
10 tasks
@neolynx
Copy link
Member

neolynx commented Sep 18, 2024

I am not a big fan of doing the lint in the same pipeline, as the system tests run for 20 minutes and they can run even if lint complains. I moved the lint to a separate check, so it can run in parallel and same me a lot of time... :-)

go version
make release
echo ${{ github.ref_name }} > VERSION
Copy link
Member

Choose a reason for hiding this comment

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

we should use make version here to have the same version

Copy link
Member

Choose a reason for hiding this comment

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

and I would move this part to the Makefile, keep it as make release so the same can be run locally or in a docker container.

@@ -53,7 +48,18 @@ jobs:

- name: "Get aptly version"
run: |
make version
make version > VERSION
Copy link
Member

Choose a reason for hiding this comment

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

the VERSION file is generated in debina/rules and the Makefile, the pipeline should avoid writing to the git repo directly I think...

@neolynx
Copy link
Member

neolynx commented Sep 18, 2024

i pulled in some of your changes to test it ... it's great to get rid of goxz, thanks a lot !

@neolynx
Copy link
Member

neolynx commented Sep 18, 2024

https://github.com/aptly-dev/aptly/actions/runs/10919177530/job/30307362665

seems it cannot find the $path somehow

@iofq
Copy link
Contributor Author

iofq commented Sep 18, 2024

https://github.com/aptly-dev/aptly/actions/runs/10919177530/job/30307362665

seems it cannot find the $path somehow

hmm seems like it's an issue with escaping slashes in the branch/ref name, we may have to replace slashes with dashes in ref_name

@neolynx
Copy link
Member

neolynx commented Sep 18, 2024

I got it somehow to build on my branch (you might wanna rebase). however the contects of the zip files seem wrong, if you wann ahave a look at it...

for the lint part, did I assume right that the whole pipeline (i.e. system tests) will not run if link fails ?

@iofq
Copy link
Contributor Author

iofq commented Sep 18, 2024

for the lint part, did I assume right that the whole pipeline (i.e. system tests) will not run if link fails ?

Yeah that was my intention, if linting is going to fail anyway then we should skip the 20 minute unit tests, but if you feel otherwise that's fine with me

@neolynx
Copy link
Member

neolynx commented Sep 18, 2024

I would like to get both lint and system tests results for PRs directly, otherwise I might have to fix lint errors before seeing if it actually works, as it happened before.

Merging will not be done with a failed lint run anyway...

@iofq
Copy link
Contributor Author

iofq commented Sep 19, 2024

updated to use makefile for release and version, zip archive should now also be formatted correctly

@neolynx
Copy link
Member

neolynx commented Sep 19, 2024

looks great ! I cherry-picked and updated slightly, the versions are now consistently based on the debian version, and release versions are used when a tag is present.

aptly_1.6.0~beta1+20240919091930.86e0512e_freebsd_arm.zip.zip

the artifacts seem to be double zipped still, and we need to check if the contents are exactly the same as before...

@neolynx
Copy link
Member

neolynx commented Sep 19, 2024

The contents of the original archives look as follows:

./aptly_1.5.0_linux_amd64:
total 28568
-rwxr-xr-x 1 lynx lynx 29231726 Jun 28  2022 aptly
-rw-r--r-- 1 lynx lynx     2268 Jun 28  2022 AUTHORS
-rw-r--r-- 1 lynx lynx     1092 Jun 28  2022 LICENSE
drwxr-xr-x 2 lynx lynx     4096 Sep 19 13:10 man
-rw-r--r-- 1 lynx lynx     4340 Jun 28  2022 README.rst

./aptly_1.5.0_linux_amd64/man:
total 52
-rw-r--r-- 1 lynx lynx 52190 Jun 28  2022 aptly.1

this is from aptly_1.5.0_linux_amd64.tar.gz. the contents of1aptly_1.5.0_darwin_amd64.zip and aptly_1.5.0_freebsd_amd64.zip look the same.

@neolynx neolynx force-pushed the feature/debianize branch 2 times, most recently from 0005fe1 to 00504b6 Compare September 19, 2024 13:58
@iofq iofq force-pushed the master branch 2 times, most recently from 6d7f78b to 4d96ebe Compare September 19, 2024 16:05
@iofq
Copy link
Contributor Author

iofq commented Sep 19, 2024

the artifacts seem to be double zipped still, and we need to check if the contents are exactly the same as before...

The double-zip is related to actions/upload-artifact#39 and doesn't end up in the final release zips like these Based on the issue there isn't a good workaround, maybe could try a different upload/download action?

The contents of the original archives look as follows:

I am happy to mirror the layout of the previous release archives; do you have interest in adding the completion files to the archives moving forward?

@neolynx
Copy link
Member

neolynx commented Sep 19, 2024

Seems to work now on my branch: https://github.com/aptly-dev/aptly/actions/runs/10944845975/job/30388887371

I got rid of the double zip by unzipping the go build output, and I added the missing files. I think the architves lok ok now.

Also the matrix build is now properly used for the debian packages, following your example. Also updated the version handling a bit, to prepare testing a release verion without CI suffix...

@iofq
Copy link
Contributor Author

iofq commented Sep 20, 2024

I got rid of the double zip by unzipping the go build output, and I added the missing files. I think the architves lok ok now.

It looks like the release doesn't include any files now. I think you can't just upload folders to the release, you need archives (the double zip doesn't happen on the actual release page)

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