Improve deployment and fix AppImage #671

TheAssassin · 2023-05-27T21:38:36Z

Contributes to #565, #670.

This pull request rewrites distribute.sh to make it easier to use, introduce building in temporary directory (to isolate build processes from one another), allows users to choose which distribution formats to build for using CLI parameters, and a lot more improvements. It also adds AppImage as an output format by integrating the previous appimagecraft setup). The AppImage also now contains a full visicut directory like the other setups, which means the extensions can be installed.

Along with the new/updated script, a Docker build script is shipped that allows for building all of the target distribution formats on any platform. The image uses Debian as a base, which ships all the necessary tools (NSIS, makepkg, checkinstall etc.).

The Arch Linux stuff doesn't seem to build in the new Dockerized setup, but it's up to someone else to test and fix that.

In my opinion, this project should focus on portable formats like AppImage and drop support for hacks like checkinstall or distribution-specific formats. Third parties can maintain distribution-specific formats, either upstream or as part of user repositories (AUR, Launchpad, ...). Let's rather make the AppImage a flawless user experience, so issues like #621 won't ever have to be worked on. The less complexity, the better.

The Inkscape plugin probably needs quite a bit more love (there's lots of old Python 2 style code, and overall it seems a bit messy), but it works now when installed from an AppImage, which is a great improvement.

This implementation removes a dependency on a third-party library. It ensures that the child process properly detaches. In comparison to the previous implementation, this new setup is compatible with AppImages. (Before, the AppImage runtime process exited prematurely.)

tools/inkscape_extension/visicut_export.py

sonatype-lift · 2023-05-27T21:41:04Z

tools/inkscape_extension/visicut_export.py

@@ -71,14 +69,16 @@ def get_single_instance_port():
                sys.stderr.write("Warning: Cannot determine session ID. please report this.\n")
    return port

-# if Visicut or Inkscape cannot be found, change these lines here to VISICUTDIR="C:/Programs/Visicut" or wherever you installed it.
+# if Visicut or Inkscape cannot be found in PATH, change these lines here to VISICUTDIR="C:/Programs/Visicut" or wherever you installed it.


E501: line too long (139 > 79 characters)

❗❗ 5 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding

File Path Line Number

tools/inkscape_extension/visicut_export.py 80

tools/inkscape_extension/visicut_export.py 349

tools/inkscape_extension/visicut_export.py 355

tools/inkscape_extension/visicut_export.py 356

tools/inkscape_extension/visicut_export.py 368

Visit the Lift Web Console to find more details in your report.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

sonatype-lift · 2023-05-27T21:41:06Z

tools/inkscape_extension/visicut_export.py

-    VISICUTBIN = which("VisiCut.exe", [VISICUTDIR])
-else:
-    VISICUTBIN = which("VisiCut.Linux", [VISICUTDIR, "/usr/share/visicut"])
+if not VISICUTBIN:


E305: expected 2 blank lines after class or function definition, found 1

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

TheAssassin · 2023-05-27T22:36:13Z

This automatic "review" is mostly nonsense. I just moved code around. I do not intend to fix less-than-optimal code from before in this PR. This can be done in future PRs.

t-oster · 2023-05-27T22:49:51Z

Looks good to me at first glance. You forgot to chmod +x the distribute-docker.sh, but that's a minor.
In order to build the arch-package (makepkg) on ubuntu there is a whole lot of stuff involved (see https://gitlab.com/t-oster/visicutbuildservice/-/blob/master/Dockerfile), but since most arch users would build from the AUR anyway instead of downloading binary packages, we should probably just ditch the archlinux build and add an automated upload of the PKGBUILD to the AUR later.

TheAssassin · 2023-05-27T22:52:15Z

Pretty much my thoughts as well. I'll drop them in a commit. We can rebase before merging.

I'm not sure how popular the .debs are, but honestly, the current checkinstall setup is a bad hack. It works to some extent, but I think binaries should be built by the distributions if any. How about dropping them as well?

t-oster · 2023-05-27T22:53:05Z

Also your version detection won't work because it's not in the project directory. We should sourround it with pushd/popd

t-oster · 2023-05-27T22:58:10Z

Pretty much my thoughts as well. I'll drop them in a commit. We can rebase before merging.

I'm not sure how popular the .debs are, but honestly, the current checkinstall setup is a bad hack. It works to some extent, but I think binaries should be built by the distributions if any. How about dropping them as well?

Not sure. I don't think we are popular enough that any debian/ubuntu distro will include visicut, so people will need packages. I am not sure how good app-images feel to fresh-installed ubuntu users. My goal was to make visicut downloadable and runnable for the average user of any popular OS in the most standard way. I did not yet try to download and run the appimage on a fresh ubuntu VM, but if that feels smooth enough, we could drop it.
But as long as it works, who cares if it's a hack?

t-oster · 2023-05-27T22:59:54Z

Ok. Now I have to look at my automated build server. I guess there are some things to fix with the new distribute script, but today is too late/early.

Thanks for your contribution.

TheAssassin · 2023-05-27T23:01:51Z

Well, that was unexpected.

I don't think we are popular enough that any debian/ubuntu distro will include visicut, so people will need packages.

Just need to ask, I guess. If the packaging process was be smoother, I think they wouldn't mind. Debian for instance has got some experience with shipping Java-based packages.

Anyway, I'm certainly biased, being a maintainer of the AppImage project and author of many tools around it. We should discuss those things in an issue. Feel free to ping me if you need input.

mgmax

Although this was already merged I still have some suggestions and comments. Glad to see that things are going forward!

distribute/Dockerfile

distribute/distribute.sh

mgmax · 2023-05-28T07:31:02Z

distribute/Dockerfile

+
+ENV DEBIAN_FRONTEND=noninteractive
+
+# note: don't build with OpenJDK > 16, as this is the runtime we ship


Our DEB package indicates that it is also compatible with Java 11:
--requires 'bash,openjdk-11-jre\|openjdk-17-jre,potrace' \

Java supports backwards-compatible compilation, depending on some setting in the maven file. https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html

Please feel free to change this. I don't see a reason not to use OpenJDK 11 at this point, as it is still a supported release as far as I can see.

distribute/distribute.sh

mgmax · 2023-05-28T07:41:49Z

tools/inkscape_extension/visicut_export.py

@@ -71,14 +69,16 @@ def get_single_instance_port():
                sys.stderr.write("Warning: Cannot determine session ID. please report this.\n")
    return port

-# if Visicut or Inkscape cannot be found, change these lines here to VISICUTDIR="C:/Programs/Visicut" or wherever you installed it.
+# if Visicut or Inkscape cannot be found in PATH, change these lines here to VISICUTDIR="C:/Programs/Visicut" or wherever you installed it.


The intended meaning was: "If the extension tells you that it can not find VisiCut or Inkscape, change these lines to where your VisiCut or Inkscape is." The target audience does not necessarily know what "PATH" is.

tools/inkscape_extension/visicut_export.py

distribute/distribute.sh

mgmax · 2023-05-28T08:03:37Z

Regarding the need for a DEB package: As far as I know, it is the cleanest and best integrated installation method we can offer on Debian/Ubuntu. No need to bundle the JRE, which depends on processor architecture, glibc versions et cetera. Automatic integration into the start menu. etc.

Inkscape still has a whole bunch of issues with AppImage, so I expect that we will also have some issues and need the DEB package as a reliable fallback option.

TheAssassin · 2023-05-28T11:57:26Z

Although this was already merged

This may have been merged prematurely.

Inkscape still has a whole bunch of issues with AppImage

None of which affect third party tools packaged as AppImages themselves. As said, the Inkscape extension is working just fine with the AppImage now.

As far as I know, it is the cleanest and best integrated installation method we can offer on Debian/Ubuntu. No need to bundle the JRE, which depends on processor architecture, glibc versions et cetera. Automatic integration into the start menu. etc.

All the experience I have collected since I joined AppImage in 2017 (and having been a full-time open-source developer for years now, too), I can tell you that this is neither clean nor that bundling a JRE is a problem. I'd rather build different kinds of AppImages (which is really easy, too, the JRE bundles provide AArch64, too). Not every institution who want to use VisiCut on Linux use Debian or derivatives. For instance, two labs I know prefer openSUSE resp. Fedora.

But let's discuss the AppImage vs. native packaging question in a new issue. I'm sure that, if you try an AppImage built by the new set of scripts, you'll like what you see.

I'll send a follow-up pull request. I might make distribute.sh a Python script, though, so that the help text can be derived from the argument parser object.

TheAssassin added 8 commits May 27, 2023 23:28

Rewrite distribute.sh completely to make it usable

544ac7f

Clean up VisiCut directory detection

2498643

Clean up and improve visicut directory detection

d4cb472

Fix usage of Inkscape extension with AppImage

5711388

Use Ubuntu as a base to fix glibc compatibility

61d8105

Allow non-root Docker builds

acc96d1

Set up distribution with Docker

dde7b92

sonatype-lift bot reviewed May 27, 2023

View reviewed changes

tools/inkscape_extension/visicut_export.py Show resolved Hide resolved

sonatype-lift bot reviewed May 27, 2023

View reviewed changes

tools/inkscape_extension/visicut_export.py Show resolved Hide resolved

sonatype-lift bot reviewed May 27, 2023

View reviewed changes

TheAssassin added 2 commits May 28, 2023 00:56

Fix version detection

d82cfb7

Drop Arch Linux packaging

21e6450

Make distribute-docker.sh executable

fc533cc

t-oster merged commit d9f022b into t-oster:master May 27, 2023
1 check was pending

TheAssassin deleted the improve-deployment branch May 27, 2023 22:58

TheAssassin mentioned this pull request May 28, 2023

Set up CI/CD completely with GitHub actions #670

Closed

mgmax reviewed May 28, 2023

View reviewed changes

TheAssassin mentioned this pull request May 28, 2023

Fix last PR #673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve deployment and fix AppImage #671

Improve deployment and fix AppImage #671

TheAssassin commented May 27, 2023

sonatype-lift bot May 27, 2023

sonatype-lift bot May 27, 2023

TheAssassin commented May 27, 2023 •

edited

t-oster commented May 27, 2023

TheAssassin commented May 27, 2023

t-oster commented May 27, 2023

t-oster commented May 27, 2023

t-oster commented May 27, 2023

TheAssassin commented May 27, 2023

mgmax left a comment

mgmax May 28, 2023

TheAssassin May 28, 2023

mgmax May 28, 2023

mgmax commented May 28, 2023

TheAssassin commented May 28, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file


		ENV DEBIAN_FRONTEND=noninteractive

		# note: don't build with OpenJDK > 16, as this is the runtime we ship

Improve deployment and fix AppImage #671

Improve deployment and fix AppImage #671

Conversation

TheAssassin commented May 27, 2023

sonatype-lift bot May 27, 2023

Choose a reason for hiding this comment

sonatype-lift bot May 27, 2023

Choose a reason for hiding this comment

TheAssassin commented May 27, 2023 • edited

t-oster commented May 27, 2023

TheAssassin commented May 27, 2023

t-oster commented May 27, 2023

t-oster commented May 27, 2023

t-oster commented May 27, 2023

TheAssassin commented May 27, 2023

mgmax left a comment

Choose a reason for hiding this comment

mgmax May 28, 2023

Choose a reason for hiding this comment

TheAssassin May 28, 2023

Choose a reason for hiding this comment

mgmax May 28, 2023

Choose a reason for hiding this comment

mgmax commented May 28, 2023

TheAssassin commented May 28, 2023

TheAssassin commented May 27, 2023 •

edited