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 nightly and CI build scripts to create Windows installers #7
Conversation
f01890e
to
ef3cb32
Compare
ee8ff28
to
ebd3165
Compare
897982f
to
0396ed3
Compare
Wild! Why is the mingw container not Arch Linux based such that pacman doesnt need to be compiled? I would also expect that other packages take less time to install. Do I understand correctly that the Docker image is updated manually, and a PR on geany or G-P would download that pre-build image (or even use cached copies) instead of rolling the container every single time (including compiling pacman)? Remark #1: Generally speaking I'm not a huge fan of docker but since that doesn't run on my machine it's probably OK :-) |
Personal preference and the fact that most other CI resources are also Debian/Ubuntu based.
First, the current image available in the registry is built and uploaded by me manually. A CI job will try to pull the image from the registry, due to current permissions settings this probably works only for jobs trigger from the repository itself (master builds and PRs created from the repository itself, not forks). CI jobs which are triggered by a forked repository will not have access to the image in the registry and so fall back to build it on the fly. In theory, we could push that on the fly built image into the registry. But I consider this too risky to open doors for malicious input. I didn't make my mind up finally on how to deal with the proper image management:
It took me a few years also to see its advantages and why it might be useful. I think in this it's a quite convenient tool to have a isolated environment to build the code.
I used Arch Linux until a few months ago as well, but IMO this is no reason to use it here :). Again, I guess it won't make a relevant different except that we would have to port the existing scripts. I guess more people out there are already or can get familiar with a Debian/Ubuntu system. |
# cleanup | ||
apt-get clean && \ | ||
rm -rf /var/lib/apt/lists/* && \ | ||
yes | pacman -Scc && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--noconfirm
. yes
doesn't terminate by itself which I is generally problematic IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you did not test what you suggest?
--noconfirm
does not clean the caches as the default for the question "Do you want to remove ALL files from cache? [y/N]" is no.
In this context yes
seems to work as intended. Did you experience a certain problem?
# For more details, see build_mingw64_geany.sh where this image is used. | ||
# | ||
# Intermediate container for building pacman | ||
FROM debian:bullseye as build-pacman |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that an "intermediate container"? And what does this mean exactly, i.e. what's the advantage over building pacman in the main docker container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage is that everything necessary to build Pacman remains in the intermediate container and gets destroyed automatically once it is finished. Nothing from this container will remain in the final container except those items which you copy explicitly.
This is very common and convenient practice when building Docker images.
https://docs.docker.com/build/building/multi-stage/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know
I just thought it's a better match because it comes with pacman, but also because it is generally up-to-date w.r.t. to toolchains and wine. So we could build the Windows binaries using the latest and greatest toolchain. Debian/Ubuntu is usually several years behind and I worry a bit if that might cause trouble in the future if mingw packages require a newer toolchain (mingw is also rolling like Arch AFAIK). But it's your choice. I think the current restrictions are OK. Perhaps we could auto-update the image based on CI but I wouldn't trigger that from Geany repos but this repository (this is where the dockerfile is after all). Do we have anything to lose when giving forks access to the image, like losing some GH actions budget to it? Anyway we can discuss that later and leave it to official repos for now. |
One more (personal) reason to use Arch: You know I'm working on a flatpak version of Geany. Assuming we ever want to create such a flatpak image for official releases via CI, then I would certainly prefer Arch Linux because that has the latest flatpak tooling. But that doesn't necessarily mean mingw jobs must use Arch Linux containers as well, so still your choice. |
I don't really like that this repository knows how to build Geany and G-P. A developer might make changes in that area and might become unable to proceed because he can't get CI to work as the job needs to be adapted. A realistic example: we might want to switch to the meson build by default (or maybe only for specific platforms) and the PR to implement would be unable to fix the CI. We should probably just provide the Docker image itself and not the scripts to build Geany or packages. |
I was working on these scripts since 2021 or maybe even 2020 and had no problems with the toolchains provided by Debian, AFAIR. And please stop this too old joke of Debian being "several years" behind. It's usually rather 1-2 years and this is on purpose. After all, I guess Debian and ArchLinux could work as base and given that the current setup works with Debian, I personally see no need to change it.
Agreed. We could move builders/scripts/build_mingw64_geany_plugins.sh and its companions to the Geany resp. G-P repos. I guess I did it this way because the initial idea of all this started with renewing the nightly build scripts and over the time it turned out to be usable for CI builds as well.
Sounds good to me.
Not that I know of. To me it seems that GH actions are not billed at all for us and so there is also no budget or limit. |
Done. |
6a3b282
to
d434dc4
Compare
log "Create source tarball" | ||
cd ${GEANY_BUILD_DIR} | ||
# create a source tarball, keep .git included on purpose for Geany GIT build detection | ||
tar --transform "s,^,geany-plugins-${GEANY_VERSION}/,S" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
geany-plugins-${GEANY_VERSION} ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
# For more details, see build_mingw64_geany.sh where this image is used. | ||
# | ||
# Intermediate container for building pacman | ||
FROM debian:bullseye as build-pacman |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know
builders/Dockerfile.mingw64
Outdated
tar xf pacman-${PACMAN_VERSION}.tar.xz && \ | ||
cd /pacman-${PACMAN_VERSION} && \ | ||
meson \ | ||
--prefix /usr/local/pacman \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not set prefix to /usr/local in the first place? That would avoid having to create symlinks and potential binary-relocation issues in the future.
This is a fresh system container, /usr/local should contain nothing but the pacman installation afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to install applications into their own prefix if possible. But /usr/local should work as well.
Since the current setup works this way, I'd leave as is.
What do you mean by "potential binary-relocation issues in the future"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. If you prefer the app-exclusive prefix, then why do you copy it to /usr/local/bin, afterwards?
It appears that pacman is run from /usr/local/bin/pacman, afterwards, yet its prefix is /usr/local/pacman. This can cause trouble if the pacman binary encodes the prefix (remember that Geany isn't relocatable by default either).
Really, I would prefer to define one prefix and then also run the program from there. Anything else is asking for trouble and is just adding confusion (I am confused). Then you don't need ln -s
and LD_LIBRARY_PATH
workarounds that I find in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whateveryou wish.
Pacman is now installed into /usr/local but setting LD_LIBRARY_PATH
is still necessary or alternatively running ldconfig
to manually update the ldcache.
This is because after installing Pacman with Meson, the ldcache is not updated automatically. So we still need to take care to update the ldcache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you run ldconfig in the container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, tried it in the first place. We would need to run it in the build container as well as in the final container (as we copy only the files in /usr/local from the one to the other but no state and cache).
I don't mind whether we set LD_LIBRARY_PATH
as it is or run ldconfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling: better update the cache properly than messing with LD_LIBRARY_PATH but I don't feel strong either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
|
||
# start wine to initially create config directory | ||
RUN /usr/local/bin/mingw-w64-i686-wine hostname.exe && \ | ||
/usr/local/bin/mingw-w64-x86_64-wine hostname.exe && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to run hostname.exe twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once for wine-i686 and once for wine-x86_64. Both commands are only executed to setup the Wind environment initially for both architectures.
We need wine-x86_64 to run Geany itself and wine-i686 to execute the installer which can be built only for i686.
# install NSIS and exiftool to inspect binary metadata | ||
nsis libimage-exiftool-perl osslsigncode \ | ||
# Geany build dependencies \ | ||
python3-lxml python3-docutils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still uneasy that Geany dependencies are encoded in the infrastructure script. What happens if a PR in Geany adds new dependencies? How can that PR possibly proceed, without breaking CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/geany/geany-plugins/pull/1201/files#diff-306a1de5460f6947d8ee9383b366b8dcc6529fa5c4b18aa9523729d8456e3d0dR175 - this is the canonical place for installing necessary dependencies.
The list here is rather some sort of cache or pre-installation of what is done in each CI run to speed up the build process. If at CI job execution time newer package versions are available than at build time of the Docker image, the newer versions are used at CI job execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I only looked at the geany build script and didn't see how it installs additional dependencies. This was my concern, that the infrastructure script must know about all dependencies. But that's not a problem then, great!
"gtk_version": "${GTK_VERSION}", | ||
"gcc_version": "${COMPILER_VERSION}", | ||
} | ||
EOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who uses versions.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing yet but it might become useful when using the scripts for nightly builds to fill the table on https://www.geany.org/download/nightly-builds/.
But I'm quite undecided yet how to proceed with the nightly builds exactly and the whole Debian package builds scripts are yet to be finalized. For now, I would like to focus on the mingw64 parts to get them integrated into the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add such artifacts only when they became needed with a clear use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are gone with the whole Debian code.
} | ||
|
||
|
||
main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of this file is identical to build_debian_geany.sh. Don't you think it would be worthwhile to share code instead of duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was struggling with this as well.
However, consequently we should move the Debian build scripts to the Geany and G-P repositories as we did for the mingw64 scripts but then sharing the code between two repositories gets more complicated again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above you said you would like to focus on the mingw-parts. Might be a good idea to leave the Debian parts out of this PR for now, then, and deal with that in a separate, follow-up PR. Then we can discuss the proper place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I removed the Debian parts for now.
|
||
|
||
# codename -> suite mapping | ||
declare -A SUITES=([buster]=stable [sid]=unstable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buster -> bullseye
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks.
I added a CI workflow on this repository to build the Docker image on push events and on a weekly schedule. |
builders/Dockerfile.mingw64
Outdated
tar xf pacman-${PACMAN_VERSION}.tar.xz && \ | ||
cd /pacman-${PACMAN_VERSION} && \ | ||
meson \ | ||
--prefix /usr/local/pacman \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. If you prefer the app-exclusive prefix, then why do you copy it to /usr/local/bin, afterwards?
It appears that pacman is run from /usr/local/bin/pacman, afterwards, yet its prefix is /usr/local/pacman. This can cause trouble if the pacman binary encodes the prefix (remember that Geany isn't relocatable by default either).
Really, I would prefer to define one prefix and then also run the program from there. Anything else is asking for trouble and is just adding confusion (I am confused). Then you don't need ln -s
and LD_LIBRARY_PATH
workarounds that I find in this file.
# install NSIS and exiftool to inspect binary metadata | ||
nsis libimage-exiftool-perl osslsigncode \ | ||
# Geany build dependencies \ | ||
python3-lxml python3-docutils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I only looked at the geany build script and didn't see how it installs additional dependencies. This was my concern, that the infrastructure script must know about all dependencies. But that's not a problem then, great!
"gtk_version": "${GTK_VERSION}", | ||
"gcc_version": "${COMPILER_VERSION}", | ||
} | ||
EOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add such artifacts only when they became needed with a clear use case.
} | ||
|
||
|
||
main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above you said you would like to focus on the mingw-parts. Might be a good idea to leave the Debian parts out of this PR for now, then, and deal with that in a separate, follow-up PR. Then we can discuss the proper place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
@kugel- are you fine when I squash the commits together or do you still need the history? |
Squashing is fine |
2d9af0c
to
3859e2c
Compare
Go for it? Or is anything missing? |
The scripts, mainly start_build.sh, can be used for nightly builds and for CI builds. The Windows scripts use a Docker image containing a full cross compilation environment for mingw64-x86_64. They create fully working installer files for Geany and Geany-Plugins, optionally even signed if a certificate is provided. The Debian build scripts are yet to be tested and finalized.
3859e2c
to
e25db90
Compare
I guess I was waiting for a final "ok" and then forgot about it. Thanks for the review. I fixed a final typo and now it's going to be merged. Yay. |
The scripts, mainly start_build.sh, can be used for nightly builds
and for CI builds.
The Windows scripts use a Docker image containing a full cross
compilation environment for mingw64-x86_64. They create fully working
installer files for Geany and Geany-Plugins, optionally even signed
if a certificate is provided.
The Debian build scripts are yet to be tested and finalized.
Even though the Debian part is not yet ready, the Windows part is working fine and I plan to use it for the nightly builds soon.
The ultimative plan is to use these scripts in the CI builds for Geany and Geany-Plugins and to provide the created installer files as artifacts to download, so developers and users can easily test master or even PR builds.