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 Arduino avrdude_packing github action for git man #1540

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

mcuee
Copy link
Collaborator

@mcuee mcuee commented Oct 28, 2023

I think this needs some review and changes to be correct.

This is a modified version of the following github action script from Arduino avrdude-packing project.
https://github.com/arduino/avrdude-packing/blob/main/.github/workflows/release.yml

I have used it for a while in my fork here.
https://github.com/mcuee/avrdude-packing

With the merge of PR #1539, no extra patches are needed to use the above script.

@mcuee mcuee added the enhancement New feature or request label Oct 28, 2023
@mcuee
Copy link
Collaborator Author

mcuee commented Oct 28, 2023

@umbynos

I am not so sure if you like to see this one in the main avrdude project or not. Please comment. Thanks.

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 28, 2023

@MCUdude, @stefanrueger and @dl8dtl

The good thing about this is that we can provide static-linked avrdude binary for Linux, macOS. But I am not so sure if we want to support this or not.

1) Change the name of the script
2) Not to run for pull-request, only run when push to git main
@mcuee mcuee changed the title Add arduino_packing github action for git man -- WIP DNM Add arduino_packing github action for git man Oct 28, 2023
@mcuee
Copy link
Collaborator Author

mcuee commented Oct 28, 2023

It seems to be correct now.

The reason to run only on git push to git main and not for PR is that this build is a bit slow (using containers) compared to normal run.

And the script is anyway not correct for PR.

@mcuee mcuee changed the title Add arduino_packing github action for git man Add Arduino avrdude_packing github action for git man Oct 29, 2023
@mcuee
Copy link
Collaborator Author

mcuee commented Oct 29, 2023

BTW, no rush for this PR. It is not essential as of now.

@MCUdude
Copy link
Collaborator

MCUdude commented Oct 30, 2023

If we can release statically linked binaries with every Avrdude release, that's a good thing for those who bundle Avrdude with other things, like with the Arduino cores I'm maintaining. Previously I had to wait three month for Arduino to release their 7.2 statically built binaries.

However, we should recommend users to install though their package manager or build from source if possible.

@stefanrueger
Copy link
Collaborator

I have no real opinion on this and am happy to merge this provided we have the expertise in the team to debug problems of packaging.

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 30, 2023

If we can release statically linked binaries with every Avrdude release, that's a good thing for those who bundle Avrdude with other things, like with the Arduino cores I'm maintaining. Previously I had to wait three month for Arduino to release their 7.2 statically built binaries.

With this PR, we will be able to generate statically linked binaries with every push to git main.

However, we should recommend users to install though their package manager or build from source if possible.

Yes I agree.

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 30, 2023

I have no real opinion on this and am happy to merge this provided we have the expertise in the team to debug problems of packaging.

Yes that is the worry. I do not have the expertise myself.

On the other hand, the code is pretty much the same as the following file maintained by @umbynos (just some minor simplification).
https://github.com/arduino/avrdude-packing/blob/main/.github/workflows/release.yml

The script depends on the container by Arduino crossbuild project, again maintained by @umbrynos. We will rely on the improvement of the container to add more functionalites sometimes (but not often, unless we add more dependancies like libserialport in the future).
https://github.com/arduino/crossbuild/pkgs/container/crossbuild

That is why I'd really like to hear the opinion from @umbynos.

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 30, 2023

@umbynos

Please help here. Thanks.

  1. Please state your opinion to see if it is good to have this PR in avrude main git repo or not.
  2. If yes, please also look at the script (basically your script with minor modification) to see if it is correct or not and if it can be simplified or improved. I have tested this myself and it seems to work. But I think it can be simplified since it is now part of the avrdude git main repo.
  3. If no, I think I may have to drop this PR.

Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

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

Hello,

Firstly, it would be an honor to see some of my work in the main avrdude repo.
Overall IMHO looks good, I've made some minor suggestions.

Previously I had to wait three month for Arduino to release their 7.2 statically built binaries.

I would like to be more responsive on this, but I'm not the one deciding priorities in Arduino... Integrating this would mean no waiting is required 😉

The only "problem" I see is this one:

The script depends on the container by Arduino crossbuild project, again maintained by @umbynos. We will rely on the improvement of the container to add more functionalites sometimes (but not often, unless we add more dependancies like libserialport in the future).

Of course improvements will be made, but I cannot make promises on the timeline... Of course libraries can be installed/compiled insiede the container at runtime, so I don't see that as a big problem. Overall I see this situation as an improvement

.github/workflows/arduino_packing.yml Show resolved Hide resolved
.github/workflows/arduino_packing.yml Outdated Show resolved Hide resolved
@MCUdude
Copy link
Collaborator

MCUdude commented Oct 30, 2023

I would like to be more responsive on this, but I'm not the one deciding priorities in Arduino... Integrating this would mean no waiting is required 😉

@umbynos sorry, that was not meant to sound like critique! I'm very grateful that you found some time to release 7.2 as statically linked binaries. I don't expect you or Arduino to always have time to spare when a new version of Avrdude gets released, but it has been very helpful in the past. But as you pointed out, if this PR gets merged, we (and other 3rd parties) can just use official statically linked binaries instead.

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 30, 2023

Just to play safe, I have created a temporary branch here to test this out. It seems to work.
https://github.com/avrdudes/avrdude/blob/test_pr1540/.github/workflows/arduino_packing.yml

I use the test branch to test and that is why it is upon push to the test_pr1540 branch.
You can see github action here.
https://github.com/avrdudes/avrdude/actions/runs/6692557883

This test_pr1540 branch will be deleted later.

@umbynos
Please carry out one more review for my latest changes. Thanks a lot for the help!

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 30, 2023

Of course libraries can be installed/compiled insiede the container at runtime, so I don't see that as a big problem.

@umbynos
Good point. Thanks.
We may need your expertise here. :-)

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 31, 2023

@stefanrueger

I think this PR is ready to be merged. But we can wait for your next merge window.

It is probably better to use "squash and merge" when you merge this PR. Thanks.

@MCUdude
Copy link
Collaborator

MCUdude commented Oct 31, 2023

I think this PR is ready to be merged. But we can wait for your next merge window.

I'm all for it, as it makes my job of maintaining 3rd part Arduino cores much easier when I want to bundle the latest Arduino version. However, we should give @dl8dtl time to share his thoughts as well.

@mcuee
Copy link
Collaborator Author

mcuee commented Oct 31, 2023

I think this PR is ready to be merged. But we can wait for your next merge window.

I'm all for it, as it makes my job of maintaining 3rd part Arduino cores much easier when I want to bundle the latest Arduino version. However, we should give @dl8dtl time to share his thoughts as well.

That is a good point.

@dl8dtl
Please share your thoughts. Thanks.

@mcuee
Copy link
Collaborator Author

mcuee commented Nov 1, 2023

Just another test to confirm that it is working, using the test_pr1540 branch (which will be deleted after this PR gets merged).
https://github.com/avrdudes/avrdude/actions/runs/6715032983

@stefanrueger stefanrueger merged commit 8222ad8 into avrdudes:main Nov 3, 2023
10 checks passed
@mcuee mcuee deleted the avrdude_packing branch November 3, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants