These changes allow us to create future release builds for Windows for the x86_64 platform and finally use GTK3. Also since the GTK bundle creation and the NSIS installer scripts get more flexible, this helps to create fullly automated cross-compiled builds.
Changes in the bundle creation script: - the GTK (and other dependencies) bundle is now created for the x86_64 platform - the new parameter "-x" allows to run script on a Linux system using Wine, therefore it is necessary to run the post-install scripts after all packages have been extracted. - use "-Sdd" for Pacman to ignore dependencies as we resolve them manually - do not use "tar -x --xz" as Pacman nowadays also downloads .zst packages, instead just download the file and let tar choose the format automatically based on the filename - install grep with Pacman as the build and target platform are now identical - update GTK dependencies to match current packages
Changes in the NSIS installer script:
- remove hard-coded paths and installer name from NSIS installer script - This makes the paths for external resources configurable using command line flags (e.g. /DGEANY_THEMES_DIR=/something/geany-themes) and also the resulting installer filename can be set via command line flags - the "INCLUDE_GTK" command line is removed as we always include the GTK bundle - the GTK version detection was removed and now we always bundle the GTK3 specific CSS files.
Do NOT merge before 1.37 is released as we should make first a final GTK2/i686 release before switching. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2590
-- Commit Summary --
* Windows: Create GTK bundle for x86_64 and suppt non-native execution * Remove hard-coded paths and installer name from NSIS installer script
-- File Changes --
M geany.nsi.in (96) M scripts/gtk-bundle-from-msys2.sh (106)
-- Patch Links --
https://github.com/geany/geany/pull/2590.patch https://github.com/geany/geany/pull/2590.diff
@eht16 pushed 2 commits.
3ff72532e434f5fa3884ae459ef62e667f2cca9f Remove hard-coded paths and installer name from NSIS installer script 15101c26b00f8c256a690ac6c7c4f260be1bf6d0 Windows: Replace GTK2 by (upcoming) GTK4 in bundle script
Just pushed 15101c2 to replace GTK2 by GTK4 and updated PR description accordingly.
@b4n commented on this pull request.
fi
done - rmdir mingw32 + rmdir mingw64 + fi +} + +delayed_post_install() { + if [ "$run_pi" ]; then + echo "Execute delayed post install tasks" + # Commands have been collected manually from the various .INSTALL scripts
This seems quite fragile to me, isn't it kind of risky to have to maintain that on our own? Also, was running individual post-install script just too slow, or other problem? One obvious solution if speed was not the issue but e.g. dependency order or something would be to run them all after (like, here).
@b4n commented on this pull request.
ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1
else - pacman -Sp mingw-w64-${ABI}-${1} + # -dd to ignore dependencies as we listed them already above in $packages
curious about why do we do that manually now?
@b4n commented on this pull request.
@@ -1,43 +1,51 @@
-#!/bin/sh +#!/bin/bash
why? I didn't test, but didn't spot any obvious bashisms, are there any (really) required?
@eht16 commented on this pull request.
fi
done - rmdir mingw32 + rmdir mingw64 + fi +} + +delayed_post_install() { + if [ "$run_pi" ]; then + echo "Execute delayed post install tasks" + # Commands have been collected manually from the various .INSTALL scripts
Yes, it is fragile. But no more than having them executed during package extraction: ``` Running post_install script for "glib2" .INSTALL: line 3: pwd: -W: invalid option pwd: usage: pwd [-LP] /build sed: can't read /bin/gdbus-codegen-script.py: No such file or directory sed: can't read /bin/glib-genmarshal-script.py: No such file or directory sed: can't read /bin/glib-mkenums-script.py: No such file or directory sed: can't read /bin/gtester-report-script.py: No such file or directory ``` They use `pwd -W` which is a MSYS special option and exist in no other `pwd` implementation.
And it continues: ``` Running post_install script for "fontconfig"
Fontconfig configuration is done via /mingw64/etc/fonts/conf.avail and conf.d. Read /mingw64/etc/fonts/conf.d/README for more information.
updating font cache... .INSTALL: line 13: mingw64/bin/fc-cache: No such file or directory done. Running post_install script for "gdk-pixbuf2" .INSTALL: line 2: mingw64/bin/gdk-pixbuf-query-loaders: No such file or directory Running post_install script for "hicolor-icon-theme" .INSTALL: line 9: mingw64/bin/gdk-pixbuf-query-loaders.exe: cannot execute binary file: Exec format error Running post_install script for "adwaita-icon-theme" .INSTALL: line 9: mingw64/bin/gdk-pixbuf-query-loaders.exe: cannot execute binary file: Exec format error ``` Many of the scripts don't work properly and much of the stuff in there we don't need. We could maybe patch the install scripts so that they work with `wine` and replace `pwd -W` but this feels even more fragile.
@eht16 commented on this pull request.
@@ -1,43 +1,51 @@
-#!/bin/sh +#!/bin/bash
There are a couple of `$(command...)` in there even though I might added them after I set it to bash :+1: The reason is this way we don't need to care whether to use bashisms or not. And shell portability isn't an issue for this particular script. It is used in a Docker container where bash is always available (as defined by Dockerfile) or it is used on MSYS2 where bash is an essential package. So we just make our own life easier. I'm a big fan of portable scripts but here it would be simply overkill.
@eht16 commented on this pull request.
ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1
else - pacman -Sp mingw-w64-${ABI}-${1} + # -dd to ignore dependencies as we listed them already above in $packages
Even though I can't reproduce it right now, I had `pacman -Sp mingw-w64-x86_64-glib2` printing also all dependencies of the "glib2" package and so we ended up with `mingw-w64-x86_64-python` and many many more. Basically this is nice but the Mingw packages have way more dependencies than we need and to keep the resulting bundle as small as possible ignore the package dependencies and define them manually.
@b4n commented on this pull request.
@@ -1,43 +1,51 @@
-#!/bin/sh +#!/bin/bash
`$(…)` is no bashism :)
But OK, your answer fair enough, color me convinced.
@b4n commented on this pull request.
ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1
else - pacman -Sp mingw-w64-${ABI}-${1} + # -dd to ignore dependencies as we listed them already above in $packages
OK. I guess so long as everything runs smoothly it's probably OK anyway indeed
@b4n commented on this pull request.
fi
done - rmdir mingw32 + rmdir mingw64 + fi +} + +delayed_post_install() { + if [ "$run_pi" ]; then + echo "Execute delayed post install tasks" + # Commands have been collected manually from the various .INSTALL scripts
Ouch. OK then, makes sense and it seems actually *more* reliable that way.
@eht16 pushed 1 commit.
ec6b20ec55bb03b72c4b195f857d660e8b058701 Merge branch 'master' into windows_full_cross_build
@eht16 pushed 4 commits.
f0173e9eadfbc38e11496c8ea6b81f49e7c87226 Windows: Ignore package signatures on bundle creation 8507f123157b2eb1d80a4f5ae8f20da6a8659d37 Windows: Ignore Pacman cache on bundle creation d63756f05c518699a0bde7ab289ca38ea71d9df9 Windows: Compile and include GSettings schemas on bundle creation 5fc652454cd0f8a67271a9963bf1e45f85628884 Windows: Update indirect dependencies for bundle creation
@eht16 pushed 1 commit.
f472e008b5ce3175f76db800c70005014a698205 Windows: Install development files by default with "Full" install target
@eht16 pushed 8 commits.
c6b08f21e77c1356248a1b1d142b6abe134cae36 Windows: Create GTK bundle for x86_64 and suppt non-native execution a2b125b76848f143b49fcbdbcea3e7980deb41d5 Remove hard-coded paths and installer name from NSIS installer script b9dd79b6e5d40ddd46fc0598df4ab0fe52413cbc Windows: Replace GTK2 by (upcoming) GTK4 in bundle script 4af8f8ac0bdb8df67dfaa69e3d1ba33d7a8f0bf5 Windows: Ignore package signatures on bundle creation 398811444231eefdfd8ce20af3e9a165c2992ff8 Windows: Ignore Pacman cache on bundle creation cfa51b8948dbd924c59518c9afebeb904509745b Windows: Compile and include GSettings schemas on bundle creation d4982871a628463142caf143589797be5d273a01 Windows: Update indirect dependencies for bundle creation 1b8c828a51a0c5ad735635fb2402514a48cdff78 Windows: Install development files by default with "Full" install target
@eht16 what about this? 1.38 is soon to be released and this PR is on the list (and part of the "remove gtk2" project)
This PR is actually release-critical, without we cannot create Windows installers. So it need to be merged before 1.38. I just did not yet merge it to give potential reviewers a chance. But since the release date is approaching, I'm going to merge it soon.
@kugel- commented on this pull request.
I find the mentions of gtk4 weird as it suggest we have any support for it which is not true.
@kugel- commented on this pull request.
Can you explain this change a bit?
@kugel- commented on this pull request.
Why ??
I've had a very coarse l look and found no blocker. The changes are generally not explained very well though (the WHY).
Sad that the "non_gtk" installier is dropped. I suppose it's hard to support with the msys2 bundle script (that I created in a distant past myself)?
@kugel- not sure whats going on, but your comments don't show in the code here, making the "Why" a bit hard to answer since its not clear what you are referring to?
I commented on the commits themselves, sorry. No idea how you're supposed to discover that, sorry, github simply sucks.
I commented on the commits themselves, sorry. No idea how you're supposed to discover that, sorry,
I can't find them in the commits either, but b4n's resolved comments show. Did you finish the review?
github simply sucks.
+1 but the rest are worse.
How can I "finish the review"? I clicked "submit review" after each comment.
Ahhhh, IIUC a review has both a set of code comments and a final "top level" comment that is not connected to code that allows the reviewer to make general comments. All the code comments and the top level comment are connected as a "review". I think when you submitted the comments as reviews they are each the final comment, not code comments and the result is a set of reviews without code comments.
When you click on a line to leave a code comment there should be "start a review" button, and then when you click on other lines lines you get "add to review" and there is a "finish review" button in the upper right where you submit it with the "top level" comment.
@kugel- commented on this pull request.
@@ -64,17 +68,25 @@ handle_command_line_options() {
"-3") gtkv="3" ;; + "-4") + gtkv="4"
I find the mentions of gtk4 weird as it suggest we have any support for it which is not true.
@@ -104,6 +104,8 @@ handle_command_line_options() {
}
The patch clearly doesn't match the commit message
@@ -135,7 +137,8 @@ _getpkg() {
if [ "$use_cache" = "yes" ]; then package_info=$(pacman -Qi mingw-w64-$ABI-$1) package_version=$(echo "$package_info" | grep "^Version " | cut -d':' -f 2 | tr -d '[[:space:]]') - ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1 + # use @(gz|xz|zst) to filter out signature files (e.g. mingw-w64-x86_64-...-any.pkg.tar.zst.sig) + ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-*.tar.@(gz|xz|zst) | sort -V | tail -n 1
With `*.tar.{gz,xz,zst}` it should be possible to do without extglob shell option.
@elextr commented on this pull request.
@@ -64,17 +68,25 @@ handle_command_line_options() {
"-3") gtkv="3" ;; + "-4") + gtkv="4"
Having the ability to choose a GTK4 build is probably useful for when "somebody" (:tm: @eht16) starts to add GTK4 support. For once Windows is leading not trailing :grin:
@eht16 commented on this pull request.
@@ -64,17 +68,25 @@ handle_command_line_options() {
"-3") gtkv="3" ;; + "-4") + gtkv="4"
Renaming GTK2 to GTK4 was done to keep the structure in the script, I assume it's not that unlikely that GTK4 will come at some point.
Considering the target audience of this script, I don't see a big source of confusion here. The script is used only by very few people after all (you and me included).
@eht16 commented on this pull request.
@@ -104,6 +104,8 @@ handle_command_line_options() {
}
I cannot see which patch and commit message you are referring to.
@eht16 commented on this pull request.
@@ -135,7 +137,8 @@ _getpkg() {
if [ "$use_cache" = "yes" ]; then package_info=$(pacman -Qi mingw-w64-$ABI-$1) package_version=$(echo "$package_info" | grep "^Version " | cut -d':' -f 2 | tr -d '[[:space:]]') - ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1 + # use @(gz|xz|zst) to filter out signature files (e.g. mingw-w64-x86_64-...-any.pkg.tar.zst.sig) + ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-*.tar.@(gz|xz|zst) | sort -V | tail -n 1
For me, it doesn't work: ```bash enrico@endor:/tmp/shglob$ ls test-1.tar.zst enrico@endor:/tmp/shglob$ ls test-*.tar.{gz,xz,zst} ls: cannot access 'test-*.tar.gz': No such file or directory ls: cannot access 'test-*.tar.xz': No such file or directory test-1.tar.zst enrico@endor:/tmp/shglob$ shopt -s extglob enrico@endor:/tmp/shglob$ ls test-*.tar.@(gz|xz|zst) test-1.tar.zst ```
`*.tar.{gz,xz,zst}` tries to expand to all of the patterns and so fails for missing files. The `extglob` variant only matches for *existing* files. I don't see a big issue here, we already require `bash` anyway and performance is also not that relevant as probably nearly all other operations in the script are way more expensive.
@b4n commented on this pull request.
@@ -135,7 +137,8 @@ _getpkg() {
if [ "$use_cache" = "yes" ]; then package_info=$(pacman -Qi mingw-w64-$ABI-$1) package_version=$(echo "$package_info" | grep "^Version " | cut -d':' -f 2 | tr -d '[[:space:]]') - ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1 + # use @(gz|xz|zst) to filter out signature files (e.g. mingw-w64-x86_64-...-any.pkg.tar.zst.sig) + ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-*.tar.@(gz|xz|zst) | sort -V | tail -n 1
if it is really a concern, grep can be your friend. Either `| grep -E '[.](gz|xz|zst)$'`, or the possibly simpler and safer `| grep -v '[.]sig$'`.
PS: or, as I wouldn't miss an opportunity of dropping some sed in: `| sort -rV | sed '/[.]sig$/d;q'` ;)
@eht16 commented on this pull request.
@@ -135,7 +137,8 @@ _getpkg() {
if [ "$use_cache" = "yes" ]; then package_info=$(pacman -Qi mingw-w64-$ABI-$1) package_version=$(echo "$package_info" | grep "^Version " | cut -d':' -f 2 | tr -d '[[:space:]]') - ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1 + # use @(gz|xz|zst) to filter out signature files (e.g. mingw-w64-x86_64-...-any.pkg.tar.zst.sig) + ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-*.tar.@(gz|xz|zst) | sort -V | tail -n 1
In what way would be these variants better? For me, they are harder to read and understand than the commented `*.tar.@(gz|xz|zst)` variant. As already said, requiring a bash extension, is not an issue in this context.
Merged #2590 into master.
@b4n commented on this pull request.
@@ -135,7 +137,8 @@ _getpkg() {
if [ "$use_cache" = "yes" ]; then package_info=$(pacman -Qi mingw-w64-$ABI-$1) package_version=$(echo "$package_info" | grep "^Version " | cut -d':' -f 2 | tr -d '[[:space:]]') - ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-* | sort -V | tail -n 1 + # use @(gz|xz|zst) to filter out signature files (e.g. mingw-w64-x86_64-...-any.pkg.tar.zst.sig) + ls $cachedir/mingw-w64-${ABI}-${1}-${package_version}-*.tar.@(gz|xz|zst) | sort -V | tail -n 1
I'm not familiar with that syntax, but sure it's probably as readable as the grep variant, and it's fine regarding your remarks
github-comments@lists.geany.org