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

Update WAF to version 2.0.26 #732

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

hedayat
Copy link
Member

@hedayat hedayat commented Oct 1, 2023

Fixes Python 3.12 compatibility
Fixes #731

@github-actions
Copy link

github-actions bot commented Oct 1, 2023

Automatically generated build artifacts for commit c66ccdf (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

Copy link
Member

@aquaherd aquaherd left a comment

Choose a reason for hiding this comment

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

./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?

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

@hedayat
Copy link
Member Author

hedayat commented Nov 14, 2023

./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
Copy link
Member

Please review the following commit on master:

commit 0b2eb08a8bac3ec4f65079e506611066c49e71dd
Author: ederag <[email protected]>
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
Copy link
Member

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

commit 74d9f85eebb13217d90fb0a8af101ded4241bd05
Author: ederag <[email protected]>
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
Copy link
Contributor

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

@hedayat
Copy link
Member Author

hedayat commented Nov 17, 2023

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
Copy link
Member

matthijskooijman commented Nov 18, 2023

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 #252 (comment)). 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:

  • Add waflib/LICENSE file
  • Remove packed code from waf script
  • Document update procedure in commit message

@matthijskooijman matthijskooijman mentioned this pull request Nov 18, 2023
@matthijskooijman
Copy link
Member

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

@hedayat
Copy link
Member Author

hedayat commented Nov 18, 2023

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
Copy link
Member Author

hedayat commented Nov 18, 2023

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 #252 (comment)).

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 hedayat force-pushed the fix-python-3.12-compat branch from c66ccdf to 1134e8c Compare November 18, 2023 20:27
@hedayat
Copy link
Member Author

hedayat commented Nov 18, 2023

OK, it should be ready I think.

Copy link

Automatically generated build artifacts for commit 1134e8c (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

* Replacing ./waf & waflib/ with the latest versions in the WAF package
* Removed binary waf footer; using `sed -i '/^#==>$/,$d' waf`
* Added waflib/LICENSE file from project LICENSE file (currently, not
  available in 2.0.26, but in their source repository:
  https://gitlab.com/ita1024/waf/-/blob/master/LICENSE?ref_type=heads)

This fixes projecthamster#731
Copy link
Member

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

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

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.

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).

So I'll merge this as soon as the CI completes.

Thanks!

Copy link

Automatically generated build artifacts for commit fc787c5 (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 aquaherd merged commit 596b88e into projecthamster:master Nov 19, 2023
4 checks passed
@aquaherd
Copy link
Member

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

@FelixSchwarz
Copy link
Contributor

Personally I like meson best.

@matthijskooijman
Copy link
Member

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
Copy link
Member

aquaherd commented Nov 19, 2023

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
Copy link
Member Author

hedayat commented Nov 19, 2023

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
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

Add Python 3.12 support
4 participants