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

Release with GitHub actions #710

Merged
merged 9 commits into from Mar 24, 2024

Conversation

TheAssassin
Copy link
Contributor

Fixes #670.

This PR introduces a GitHub actions workflow to create releases on GitHub with all the available binaries.

It is intended to replace the external VisiCut build server and make it easier for contributors to maintain their own builds without the need for external infrastructure and complicated setup procedures.

Please note that the second commit is a workaround for an NSIS bug. The generated license file contains non-ASCII characters (it is UTF-8 encoded). I cannot make NSIS accept the file as-is, it claims it violates codepage 0 (to my understanding, this is the currently active codepage; on Windows, it should be some UTF-8 compatible scheme; I have no idea what the default on Debian is, though).

A demo of the generated release can be found here: https://github.com/TheAssassin/VisiCut/releases/tag/continuous
Tagged releases will generate a non-prerelease entry. The text can be customized, please see pyuploadtool's docs.

@mgmax
Copy link
Collaborator

mgmax commented Mar 5, 2024

Regarding the license UTF8 issue: I assume it worked correctly in the old build environment. What's different now? I would have expected it to work the same because it uses the same docker container as before.

@TheAssassin
Copy link
Contributor Author

I have no idea. I spent a couple of hours on this already. The software stack seems to be exactly the same. There is no newer version of NSIS in the backports, for instance. Help would be appreciated.

We could install a newer or older version into the Docker container manually if we have to.

@mgmax
Copy link
Collaborator

mgmax commented Mar 8, 2024

Looks like the distribute job does not use the VisicutBuilder docker container and therefore uses a different NSIS version.

@mgmax
Copy link
Collaborator

mgmax commented Mar 9, 2024

The distribute step is currently (almost) duplicate because build.sh already builds the packages, except for AppImage.
https://github.com/t-oster/VisicutBuilder/blob/master/build.sh#L66-L69

In the distribute step, a different build script is used (distribute-docker.sh) with a different container than VisicutBuilder. The distribute-docker.sh is missing some features of VisicutBuilder/build.sh, at least the windows-addons and macos-addons files. If you want to get rid of VisicutBuilder, that's fine for me, but then we should cover all features (or have a good reason when we drop features).

What I especially liked about VisicutBuilder was that you could locally with just one docker command build VisiCut for every platform (except AppImage), and the only dependency you needed was Docker.

@TheAssassin
Copy link
Contributor Author

We can always change to the other image if needed. It used to work with plain Debian, though. I have no idea whether VisicutBuilder is maintained regularly and given the fact that once the external CD setup is obsolete there is no use for the image outside of this repository, that'll eliminate another dependency. Of course, one can just copy the Dockerfile from VisicutBuilder to this repopsitory, too.

The idea of the distribute step is to run the builds in parallel to a) speed things up and b) see on the actions dashboard which deployment failed (resp. whether it's a build problem if all of them failed).

distribute-docker.sh is just as easy to use as VisicutBuilder. Personally, I foud VisicutBuilder quite a bit more unintuitive because it "just runs" and "does things". The script has a help text and it also just runs a single build if needed. In local development, as long as the Dockerfile (or the base image) don't change, it's pretty much as fast as using distribute.sh directly.

I'm open to changes, please let me know what you'd like. In my opinion, the less external dependencies, the better.

@TheAssassin
Copy link
Contributor Author

So I updated the branch, but unfortunately the image does not work out of the box. I'm undecided on whether it's worth improving/fixing the VisicutBuilder image, or whether it's just easier to copy the necessary portions of the VisicutBuilder Dockerfile...

@t-oster
Copy link
Owner

t-oster commented Mar 11, 2024

If it has the same functionality and usability in the end it's better to have everything in one repository IMHO

@TheAssassin
Copy link
Contributor Author

Mind to elaborate a bit please? I need to understand your expectations and requirements. Do you plan on giving up on the external CI then?

@t-oster
Copy link
Owner

t-oster commented Mar 11, 2024

Well the build process should work from command line Linux with just docker installed and should be able to build all distributions. Then there would not be any need for an external visicutbuilder image. And if the GitHub distributions work, I would be fine with removing download.visicut.org if there are no objections

@t-oster
Copy link
Owner

t-oster commented Mar 11, 2024

However since @mgmax is mostly maintaining visicut at the moment and I am very inactive, I would say it's his decision

@TheAssassin
Copy link
Contributor Author

I see. That's provided with distribute/distribute-docker.sh and the reason I added that script along distribute.sh. Things should "just work" and they should work locally, too (especially for debugging/testing). I have not run a special test, but I'd expect it to work in a random live CD VM with Docker installed.

I'll try to fix the build issue as proposed in #710 (comment) by copying stuff from VisicutBuilder.

@TheAssassin
Copy link
Contributor Author

I just copied @t-oster's fix for the NSIS bug from VisicutBuilder and added the remaining package dependences (even though I'm not sure what they're exactly used for, for potrace especially I don't see any use at this point but it doesn't really hurt either).

@t-oster could you confirm please whether potrace and the macOS/Windows "addons" are still needed anywhere? They don't seem to play any part in the current build process and seem to be leftovers. I think we can just leave them out.

@t-oster
Copy link
Owner

t-oster commented Mar 11, 2024

well potrace is used for the bitmap-to-vector conversion when you use VisiCut with a camera and take a picture of what you want to cut/engrave, you can just select it and create a shape out of that image.

@TheAssassin
Copy link
Contributor Author

So it's a runtime dependency. It's not shipped at the moment from the binaries built by distribute.sh (not sure if it ever had been). The Debian package suggests its installation at least. We'd have to add this. However, that's for another PR, and should be tracked in a separate issue.

That said, I'm not sure that feature (and the built-in vectorization of raster graphics VisiCut appears to provide) are really worth keeping in the software. Personally, I use (and prefer) to use Inkscape for editing input files. I'd like to propose removing those features to focus on the core functionality and will open an issue for that.

The build has passed, i.e., this PR is ready for merging! Feel free to squash-merge. Otherwise, please notify me and I'll clean up the history.

@t-oster
Copy link
Owner

t-oster commented Mar 11, 2024

yes, it's a runtime dependency and for debian we just declare it, but for osx and windows we bundle the binaries. It pretty much depends on your workflow, but if you use VisiCut with a camera attached to the lasercutter it is IMHO a good feature, because you don't have to switch software for some simple tasks and even children can draw their ideas onto paper, take a picture in visicut and cut it. Not sure how many people use it though.

@TheAssassin
Copy link
Contributor Author

Since I don't have a camera attached (yet), I have never tested this, so I cannot really tell how well it works. I have access to a (proprietary, of course) Shaper Trace that works reasonably well. But as said, this is for another day. We should open an issue.

@mgmax
Copy link
Collaborator

mgmax commented Mar 12, 2024

My suggestion would be:

  • Accept that the vectorization feature will be broken on Windows and Mac for now, maybe remove it later (separate discussion)
  • Remove the unneeded build step because distribute builds the stuff (again) anyway. Then we no longer depend on VisicutBuilder.
  • Remove download.visicut.org once the new release stuff is working successfully for a few months
  • Use a fixed release/hash of pyuploadtool and appimagecraft (I am no friend of automatically changing external dependencies)

@TheAssassin
Copy link
Contributor Author

Use a fixed release/hash of pyuploadtool and appimagecraft (I am no friend of automatically changing external dependencies)

Will do, if "latest stable release" isn't stable enough for you. But then I suggest you subscribe to both repositories to avoid missing updates.

@TheAssassin
Copy link
Contributor Author

Friendly reminder, this is ready.

@mgmax mgmax merged commit 8797012 into t-oster:master Mar 24, 2024
6 checks passed
@mgmax
Copy link
Collaborator

mgmax commented Mar 24, 2024

Thank you for bringing this forward.

In my personal matter of taste, it could be developed further towards a trusted/reproducible build. In this PR we have introduced at least three more external URLs that are loaded without verifying a sufficiently strong checksum. Two in the GH Actions build and one indirectly in in https://github.com/TheAssassin/appimagecraft/blob/6b36fda05d50b356024bd3b12ca426ba73fe334b/appimagecraft.yml#L13 . For comparison, the existing distribute.sh script checks the hash of the downloaded JRE before extracting it.

echo "$hash $downloaded_file" | sha256sum -c

I will open a separate issue for that.

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.

Set up CI/CD completely with GitHub actions
3 participants