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

msys2 bundle script: Rework for less typo fragility, and add more pac… #560

Closed
wants to merge 2 commits into from

Conversation

kugel-
Copy link
Member

@kugel- kugel- commented Jul 11, 2015

…kages

The packags are neded due to recursive dependencies

cairo: $CAIRO
adwaita $ADW
$gtk: $GTK
read -d' ' pkgs <<EOF
Copy link
Member

Choose a reason for hiding this comment

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

instead of read, you could also simply use

pkgs="
libwinpthread
gcc-libs
libiconv
...
"

Works the same in the for loop.

But if you like the read better I don't mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasnt sure if multi line variables isnt some bash extension

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem to be (dash likes it just fine too)

@b4n
Copy link
Member

b4n commented Jul 11, 2015

LGTM

@kugel-
Copy link
Member Author

kugel- commented Jul 11, 2015

As noted by @eht16 there's still two packages missing

@kugel-
Copy link
Member Author

kugel- commented Jul 11, 2015

I changed the pkg= assignment, but left the files variable alone. Also: post_install message is only shown when actually doing it.

@eht16
Copy link
Member

eht16 commented Jul 12, 2015

Now we've got all necessary DLLs for the runtime.
But I have to build Geany against this bundle as well since there are binary incompabilities. Not that surprising and so it's ok.
Though while trying to compile Geany, I noticed at least gdk-pixbuf is missing from the bundle.

@eht16
Copy link
Member

eht16 commented Dec 21, 2015

We should merge this soon as the time slot to the next release is again getting smaller.

I noticed we need even more packages in order to be able to run Geany from the created GTK bundle:

For GTK2 and GTK3:
pixman
gdk-pixbuf2

And for GTK3 additionally:
libepoxy

Not sure why we didn't need or notice these ones last time, maybe they are new dependencies due to changed builds or versions or whatever.

Furthermore, I changed the script slightly to operate more explicitly when moving the extracted package contents:

@@ -109,9 +115,14 @@

 if [ -d mingw32 ]; then
        for d in bin etc include lib locale share; do
-               rm -rf $d
-               mv mingw32/$d .
+               if [ -d "mingw32/$d" ]; then
+                       rm -rf $d
+                       mv mingw32/$d .
+               fi
        done
+       rm -rf mingw32/var/cache/fontconfig
+       rmdir mingw32/var/cache
+       rmdir mingw32/var
        rmdir mingw32
 fi

The first part is just checking whether the subdirectories (etc include lib locale share) actually exist before trying to mv and rm them. This prevents unnecessary error messages like for locale which doesn't seem to exist on the top-level, at least in my tests.

The second part is to remove the mingw32/var/cache/fontconfig directory which gets created but we don't need, I guess. I used multiple rmdirs on purpose here instead of a simple rm -rf to explicitly trigger an error and leave any other unexpected contents there in case they are important and would get lost unseen otherwise.

@kugel-
Copy link
Member Author

kugel- commented Jan 12, 2016

@b4n @eht16 @sardemff7 Here's an update.

The commits add another script, get-pacman-pkg-deps.py, which just parses the "pactree -g" output (that command outputs a hierarchical dependency graph). The point is to get the dependencies of gtk (2 or 3) right (all of them), in the right order. The right order is necessary so that upon extracting the package files, their post_install commands can be run in the right order.

Again, the post_install commands have to be run, otherwise the gtk bundle is incomplete. One effect of an incomplete bundle is easily visible: gtk's file open dialog outright crashes (easily reproduced with ctrl+o on a geany that runs on an incomplete bundle).

Yes, it's ugly, but I'm not seeing alternative solutions.

@codebrainz
Copy link
Member

One effect of an incomplete bundle is easily visible: gtk's file open dialog outright crashes

Why does it crash though? This sounds more like a bug in MSYS/Pacman.

@sardemff7
Copy link
Contributor

Are you sure pactree -u does not output the list in the right (or just reversed) order for running post_installs?

@kugel-
Copy link
Member Author

kugel- commented Jan 12, 2016

Why does it crash though? This sounds more like a bug in MSYS/Pacman.

Some gsettings schema is missing which created by the post_install
command. It works after running the post_install step. Pretty sure gtk
is buggy here. But even if it wouldn't crash, I expect gtk or other
libraries need the post_install step in order to work as expected.

Are you sure |pactree -u| does not output the list in the right (or
just reversed) order for running |post_install|s?

Yes, pretty sure

@eht16
Copy link
Member

eht16 commented Jan 13, 2016

I didn't test the new script yet but will do in the next few days. I'm excited.

@kugel- any reason why you ignored my changes without any comment?

Also, I wonder whether you have also a left .BUILDINFO file from glib2 laying around? On my system this file is left-over after INSTALL .MTREE .PKGINFO have been deleted.

@kugel-
Copy link
Member Author

kugel- commented Jan 13, 2016

@eht16 I didn't ignore your change. However, I added var to the directories added to the bundle so it's moved out of the temp directory. I didn't notice .BUILDINFO yet, sorry.

@eht16
Copy link
Member

eht16 commented Jan 16, 2016

There must be some differences between your and my system, I get the following output when trying to create a GTK2 bundle:
http://pastebin.geany.org/04tvt/

  • in the temporary mingw/ directory, I get no locale folder hence the message mv: cannot stat 'mingw32/locale': No such file or directory
  • the permission denied error at the end only occurrs sometimes, I can successfully prevent this when I add a sleep 1 at line 96. This might be some special bug/behaviour of my system, I'm testing the bundle creation script on a RAM disk to speed up testing a bit
  • there is a strange mkdir error on line 39 which I didn't debug yet
  • as you can see in the ls output at the end, there is this .BUILDINFO file laying around on my system. Not critical nor hard to fix but I'm curious why this doesn't happen for you

My MSYS2 system is up2date (according to pacman -Syu), I already cleared the cached packages, downloaded them again, no change.
@kugel- could you compare if your package versions are similar to those I used?

However, besides those smaller problems mentioned above, the dependency handling seems like the real issue to me:
I like the little Python script, nice idea. But it pulls in many new dependencies which are not strictly necessary I think. The following list contains new packages installed by the script which are not necessary to run Geany from the created bundle (tested):

mingw-w64-i686-libjpeg-turbo
mingw-w64-i686-gmp
mingw-w64-i686-jasper
mingw-w64-i686-xz
mingw-w64-i686-libtiff
mingw-w64-i686-lzo2
mingw-w64-i686-libxml2
mingw-w64-i686-gnome-common
mingw-w64-i686-shared-mime-info
mingw-w64-i686-adwaita-icon-theme

I'd prefer to keep the hand-written package list and extend it as necessary. It's not that the package dependencies change five times a month, I guess.
Don't get me wrong, your approach is the right one, but as long as pactree pulls in so many extra dependencies I'm a bit worried about the final bundle size which we have to distribute.

I also started already writing some code to strip the created bundle when a ZIP archive is requested (deleting gtk-doc stuff, headers, manpages, static libraries, ...) but I would wait for the dependency issue until I finish this.
The previously used bundle which we distributed had about 40 MB, the created bundle with all those dependencies has about 170 MB.

@kugel-
Copy link
Member Author

kugel- commented Jan 16, 2016

I suggest stripping the bundle only afterwards. Since the post_install scripts have to be run more dependencies might be required than runtime dependencies

@eht16
Copy link
Member

eht16 commented Jan 16, 2016

Of course, stripping would be the very last step. To be clear: I did not run the stripping code yet. The above mentioned issues happen with the unmodified scripts from this PR.

@kugel-
Copy link
Member Author

kugel- commented Jan 16, 2016

I understand that. I meant (regarding the numerous dependencies): Rather than maintain the package list by hand strip the bundle afterwards, to make sure new dependencies aren't unseen.

@eht16
Copy link
Member

eht16 commented Jan 17, 2016

Well, that would make the stripping just more complex.
I don't see so many problems with the manual dependency list, especially as you already created it.

I guess we will detect new dependencies which were missing by a fixed list quite quickly when testing the created bundle.
And we would save us things like the Adwaita icon theme for GTK2 which is huge in size but necessary for the GTK2 build.

What about the other issues?

@kugel-
Copy link
Member Author

kugel- commented Jan 17, 2016

You want to strip either way so I don't see the point. Anyway, feel free to compile the list of packages a ignore my script. Regarding adwaita, I don't think it's pulled in for GTK2.

I'll try to have a look at the other issues this week.

@codebrainz
Copy link
Member

Regarding adwaita, I don't think it's pulled in for GTK2

Just as a note/reminder, IMO I don't think we should ship official GTK+ 3 Windows release until we find an theme that at least makes an attempt to look "native". The Adwaita theme is completely intolerable in Windows, IMO. For icons, I think we can just bundle the Tango theme, which looks like what is being used in GTK+ 2 already.

@eht16
Copy link
Member

eht16 commented Jan 17, 2016

@kugel- Adwaita is pulled in as you can see in my pastebin.
http://pastebin.geany.org/WQNB6/ this is a diff to "bring back" the fixed package list, extended by libepoxy for GTK3 and pixman/pixbuf dependencies for both GTK versions.
This is just my suggestion, no idea what's the best way to go.

@codebrainz fine with me. The less builds we ship, less work. And for the user on Windows it probably doesn't matter if Geany is built against GTK2 or GTK3 as long as it works.

@kugel-
Copy link
Member Author

kugel- commented Feb 22, 2016

I guess we need to resolve this for the upcoming 1.27 release.

@eht16
Copy link
Member

eht16 commented Feb 22, 2016

Indeed.
While I'm still waiting for your feedback (see comment #560 (comment)), I started working on a companion script for Geany-Plugins based on your initial script and my above mentioned changes. It's almost complete, will probably open a PR later this week.

@eht16
Copy link
Member

eht16 commented Feb 28, 2016

My latest version of the script:
https://gist.github.com/eht16/1aad667c4a68afd8126f

Changes:

  • make getpkg() more robust against ambigous package names (for G-P I had the case where dbus and dbus-glib is in the package list, the previous shell glob ls $cachedir/mingw-w64-${ABI}-${1}-* was too relaxed and only match dbus-glib for both package names), now the package is queried with pacman and the version included into the shell glob
  • put steps into functions, trying to get the script a bit structured
  • add cleanup code to reduce the created bundle to the necessary

@eht16
Copy link
Member

eht16 commented Mar 6, 2016

My latest version of the script:
https://gist.github.com/eht16/1aad667c4a68afd8126f

Revision 3 improves some cleanup stuff and adds download_and_extract_grep_and_sort to embed grep.exe and sort.exe from UnxUtils into the GTK bundle which we used to distribute with Geany for users' convenience.

@codebrainz
Copy link
Member

@eht16 remember the bug about old-timey grep in #789 not supporting the needed features. The UnxUtils one appears to be from 2003.

@eht16
Copy link
Member

eht16 commented Mar 6, 2016

Thanks for the reminder of #789.
Actually I searched this morning (UTC) for a recent version of grep for Windows but didn't find anything. So I ended up again with grep from UnxUtils but this time a slightly newer version (2.5.1) than the version 2.4 we shipped with Geany 1.26. That newer version of grep has the --include option so it should be ok, still it's very old.

Another option would be grep from MSYS2, but it would also require to ship some runtime libraries of MSYS2 as dependencies.

@codebrainz in #789 you said, you built your own grep binary. Is building grep hard and/or would be the binary suitable for distribution?

@codebrainz
Copy link
Member

@eht16 IIRC it was very straight-forward to build Grep from source on MSYS2. I guess it will have some normal deps (libgcc, etc), but probably nothing that won't already be there from our other deps.

@eht16
Copy link
Member

eht16 commented Mar 8, 2016

Yeah, compiling grep with MSYS2 is as easy as configure && make && make install, woohoo.
The only additional dependency it creates is pcre but this is ok I guess.

Now the question is: would it be ok to include a pre-compiled grep.exe in this script or add a few lines to the script to download, unpack and build grep on the fly to make it more transparent where it comes from?
I'd vote for the latter option but it will increase runtime of the script signifanctly because compiling grep will take some time (to be exact it is rather running configure which is slow).

@codebrainz
Copy link
Member

@eht16 according to this, you can build GLib with --with-pcre=system and use the same PCRE for GLib and Grep. Not sure it's worth the hassle, but it would avoid having two different copies of PCRE library in Geany distribution.

@kugel-
Copy link
Member Author

kugel- commented Mar 8, 2016

There are also GPL issues if we actually compile grep too. just saying.

@codebrainz
Copy link
Member

@kugel- WAT?

@elextr
Copy link
Member

elextr commented Mar 8, 2016

@kugel- why is there a license problem? grep is gpl3 so we can distribute it, though we might have to point to its source.

@kugel-
Copy link
Member Author

kugel- commented Mar 8, 2016

In theory we'd have to keep the same source we used for compiling in case someone asks for it. Re-distributing an existing binary distribution doesn't have this problem (though it's also some kind of a grey area).

@elextr
Copy link
Member

elextr commented Mar 8, 2016

Isn't the source available online, where did we get it to compile? We don't have to actually make the copy available ourselves.

@kugel-
Copy link
Member Author

kugel- commented Mar 8, 2016

Yes, we have to. Just because we made no modification doesnt mean we don't have to provide the source we used to compile the binary. Pointing to another repository isn't sufficient, as far as the GPL is concerned.

@elextr
Copy link
Member

elextr commented Mar 8, 2016

The GPL v3 explicitly allows the source to be on a third parties server.

@kugel-
Copy link
Member Author

kugel- commented Mar 8, 2016

Interesting. I didn't know that.

@eht16
Copy link
Member

eht16 commented Mar 10, 2016

My latest version of the script:
https://gist.github.com/eht16/1aad667c4a68afd8126f

I added creation of a bundle information file ReadMe.Dependencies.Geany.txt (if anyone has a better name, let me know). This file lists where we downloaded the included binaries from, and for grep, also the compile command. This should make it quite transparent, hopefully this is enough.
For now, I included an URL to download.geany.org/contrib/ where we could place the grep source tarball which we used for compilation. Even if GPLv3 would allow third-party servers, this way we should be safe also and putting the source tarball there is no harm at all for us.

What do you think?

I would do the same for the Geany-Plugins bundle create script afterwards.

@kugel-
Copy link
Member Author

kugel- commented Mar 10, 2016 via email

@eht16
Copy link
Member

eht16 commented Mar 11, 2016

My latest version of the script:
https://gist.github.com/eht16/1aad667c4a68afd8126f

Example ReadMe.Dependencies.Geany.txt:
http://pastebin.geany.org/3h5Yr/

In #560 (comment) I asked whether to compile grep ourselves or not. So far I didn't see any answer except the licensing issue regarding binary redistribution (thanks for that).
Anyway, I now added a few commands to the script to download and compile grep.
While this increase the execution time of the script, it is probably the cleanest and most transparent solution.

Since we are seriously running out of time here, please hurry up with any feedback.
I'd like to create a new PR with my version of the script tomorrow to not further hijack this PR :).

eht16 added a commit to eht16/geany-plugins that referenced this pull request Mar 12, 2016
@eht16
Copy link
Member

eht16 commented Mar 12, 2016

geany/geany-plugins#375 (bundle script for Geany-Plugins) updated to create a bundle info file for Geany-Plugins.

eht16 added a commit to eht16/geany that referenced this pull request Mar 12, 2016
Update the list of dependencies, include sort.exe and grep.exe,
create a information file with all download links of included
binaries and re-structure the script for better readability.
See geany#560 for details.
@eht16
Copy link
Member

eht16 commented Mar 12, 2016

As said before, I created a new PR with my version of the script which I'm going to use for the release tomorrow: #959

eht16 added a commit to eht16/geany that referenced this pull request Mar 12, 2016
Update the list of dependencies, include sort.exe and grep.exe,
create a information file with all download links of included
binaries and re-structure the script for better readability.
See geany#560 for details.
@eht16
Copy link
Member

eht16 commented Mar 13, 2016

#959 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants