projecthamster / hamster

GNOME time tracker
http://projecthamster.org
GNU General Public License v3.0
1.08k stars 249 forks source link

Update WAF to version 2.0.26 #732

Closed hedayat closed 1 year ago

hedayat commented 1 year ago

Fixes Python 3.12 compatibility Fixes #731

github-actions[bot] commented 1 year ago

Automatically generated build artifacts for commit c66ccdf25bd0741dc24377f5ddd0d2bb773b94ce (note: these links will expire after some time):

To test this PR, download and unzip the flatpak application above and then install with:

flatpak install Hamster.flatpak
hedayat commented 1 year ago

./waf compiles a working hamster under debian stable only after installing python-is-python3, since debian installs the python executable only as python3. This is probably OK since python2 is long dead. However waf itself features a shebang of #!/bin/en/python3, ideas?

I guess a PR or issue for waf project about this is an ideal solution. We can patch it locally in the mean time too.

The deletion of LICENSE.txt comes at a surprise. Please confirm that this is OK.

Well, I just replaced the dir with that of waf, but maybe LICENSE file was manually added in the first place too. If it seems needed, I can add waf's license here too. Although, maybe adding waf's license out of waflib/ directory is a better solution so that no manual action is needed in future updates too.

aquaherd commented 1 year ago

Please review the following commit on master:

commit 0b2eb08a8bac3ec4f65079e506611066c49e71dd
Author: ederag <edera@gmx.fr>
Date:   Sat Jul 13 16:41:20 2019 +0200

    add explicit license file for waflib

    License copied on 2019-07-13 from
    https://waf.io/apidocs/copyright.html
    Same as the main waf script license header.
    Checked that this is the New BSD License (3-clause)
    https://en.wikipedia.org/wiki/BSD_licenses#3-clause
    as also stated in
    https://en.wikipedia.org/wiki/Waf#License

Is this still relevant as of today?

aquaherd commented 1 year ago

Also there is this log in the history of ./waf itself:

commit 74d9f85eebb13217d90fb0a8af101ded4241bd05
Author: ederag <edera@gmx.fr>
Date:   Wed Feb 19 21:30:08 2020 +0100

    remove binary waf footer

    sed -i '/^#==>$/,$d' waf
    This helps debian packaging
    https://github.com/projecthamster/hamster/issues/252#issuecomment-587847269
    and seems harmless.
    Signatures are checked once at waf update.
    https://waf.io/book/#_getting_the_waf_file

Please check if this is still required and if yes, apply the sed to waf and force-push, please.

FelixSchwarz commented 1 year ago

Just wanted to mention that this patch allows me to build hamster on Fedora 39 and it seems to work fine so far.

hedayat commented 1 year ago

Please review the following commit on master:

    ...
    https://en.wikipedia.org/wiki/Waf#License

Is this still relevant as of today?

Thanks, I'll take a loop. I guess it does. But, as I said, I think it'd be better to add a license file to the top level dir instead (e.g. named LICENSE.waf), so that this directory can be replaced as a whole in future updates.

Please check if this is still required and if yes, apply the sed to waf and force-push, please.

I'm not sure about this one. If someone builds Debian packages already, I guess its better for him to test, as I might not test it anytime soon. But I'll check the change to see if I've any clue why it was needed at the time, since it looks like a weird one.

matthijskooijman commented 1 year ago

Thanks, I'll take a loop. I guess it does. But, as I said, I think it'd be better to add a license file to the top level dir instead (e.g. named LICENSE.waf), so that this directory can be replaced as a whole in future updates.

I'm not entirely sure, since a top-level file somewhat pollutes the namespace, and if upstream ever changes the wording of their license file, we might carry an older version around indefinitely. I think I'd prefer to stick to a file in waflib, and add some more details to the commit message about how this update commit was created (I think that's important regardless of this license - where did we take these files from? upstream git or tarball? Were any changes made or are files copied verbatim? Which files were copied and which were omitted? etc. Ideally, this would be written as shell commands that were executed, since then it is very easy to reproduce later. Having said that, I could also live with a top-level LICENSE.waf file, if the process is documented.

What is a bit weird about the license is that the waf tarball does not have any license information in it, apart from license headers in the waf and waf-light script. It seems that upstream considers those license headers to apply to the project as a whole, but has also added a project-level LICENSE file a few weeks ago (not present in 2.0.26 yet) to clarify this. So I would suggest we take that LICENSE file from their git, and include it as waflib/LICENSE in our repo (so not LICENSE.txt as it is now, to match the upstream new filename).

I'm not sure about this one. If someone builds Debian packages already, I guess its better for him to test, as I might not test it anytime soon. But I'll check the change to see if I've any clue why it was needed at the time, since it looks like a weird on

The reason the binary packed version of waf is removed, is that Debian has a policy of not allowing binary/packed stuff in source tarballs, at least if source is available (see also https://github.com/projecthamster/hamster/issues/252#issuecomment-587847269). In this case, the waf source files are available, so it does not make sense to duplicate the same code in the packed version. Regardless of Debian, I would say it also makes sense for us to have only one copy of waf in the source tree, in case we end up making modifications, having a packed copy might lead to surprises.

So, for me, changes needed are:

matthijskooijman commented 1 year ago

Maybe also add `Closes: #731" or so to the commit message?

hedayat commented 1 year ago

Maybe also add `Closes: #731" or so to the commit message?

There is one in PR's description. I guess it works too (?!). I'm not sure actually. :P

hedayat commented 1 year ago

The reason the binary packed version of waf is removed, is that Debian has a policy of not allowing binary/packed stuff in source tarballs, at least if source is available (see also https://github.com/projecthamster/hamster/issues/252#issuecomment-587847269).

What?! :P Reading the quoted commit message, I thought it has something to do with signatures or something. What's the use of packed stuff there?! Yeah, such thing is probably not allowed in many distros. OK, I'll remove it.

hedayat commented 1 year ago

OK, it should be ready I think.

github-actions[bot] commented 1 year ago

Automatically generated build artifacts for commit 1134e8ce8fc3d4ddde23e04b9f391803c8070a69 (note: these links will expire after some time):

To test this PR, download and unzip the flatpak application above and then install with:

flatpak install Hamster.flatpak
github-actions[bot] commented 1 year ago

Automatically generated build artifacts for commit fc787c56fb86b0a25601315cbd3c30f5a3cc2406 (note: these links will expire after some time):

To test this PR, download and unzip the flatpak application above and then install with:

flatpak install Hamster.flatpak
aquaherd commented 1 year ago

Just a side note: I hear from you guys that waf is deprecated. Which build system would you recommend instead?

FelixSchwarz commented 1 year ago

Personally I like meson best.

matthijskooijman commented 1 year ago

I'm not sure it is, on https://waf.io/ the latest release is from august this year, so it seems it is still developed.

Often, python software is installed from pypi using pip/pipx, which has various underlying tools, but usually those are limited to python code only, while hamster also needs dbus services, gsettings schemas, etc.

I recall investigating this in the past, but now I cannot find any record of that (I thought there would be an issue about this, but I cannot find it). If we want to further discuss this, maybe create an issue for it?

aquaherd commented 1 year ago

OK I will create an issue.

Meson would be my favourite too but I only ever used it for C/C++. Is it any good for packaging python?

hedayat commented 1 year ago

Ah, missed that. It should work to autoclose the issue (you can preview this with the "Successfully merging this pull request may close these issues." list in the right bar in the github UI), but I prefer also putting it into the commit message, since then the git history has a direct link to the issue, instead of only via the pull request. Just a nitpick, but I've gone ahead and made this change to your branch.

Well, I thought PR's description will be added to merge commit's message, but apparently it doesn't happen on GitHub (it does in GitLab). Thanks anyway.

I've also rebased on top of master for a cleaner merge history, force pushing to your branch.

Your other changes look good to me too!

Additionally, I've:

* Tested that without this PR, waf breaks on py3.12

* Tested that with this PR, things, waf works on py3.12

* Verified that the files added by this commit are indeed verbatim from the waf 2.0.26 tarball

* The files installed by the old waf with py3.11 are identical to the ones installed by the new waf with py3.12 (i.e. verified no behavior changed unintentionally).

Thanks!

matthijskooijman commented 1 year ago

Well, I thought PR's description will be added to merge commit's message, but apparently it doesn't happen on GitHub (it does in GitLab). Thanks anyway.

Good point, I thought so as well, but it looks it only added the title to the merge commit.