Page MenuHome
Differential D10629

Python version of `make_source_archive.sh`
ClosedPublic
Actions

Authored by Sybren A. Stüvel (sybren) on Mar 5 2021, 6:01 PM.

Details

Reviewers
Campbell Barton (campbellbarton)
Dalai Felinto (dfelinto)
Commits
rB8771f015f50e: Python version of `make_source_archive.sh`
Summary

This is a Python version of the existing make_source_archive.sh script. IMO it's easier to read, and it'll also be easier to extend with the necessary functionality for D10598: Fix T86124: Self-hosting external libraries packages.

The number of lines of code is larger than make_source_archive.sh, but it has considerably less invocations of awk ;-) And also the filtering is integrated, instead of forking out to Python to prevent certain files to be included in the tarball.

Diff Detail

Repository
rB Blender
Branch
temp-make-source-archive
Build Status
Buildable 13330
Build 13330: arc lint + arc unit

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Mar 5 2021, 6:01 PM
Sybren A. Stüvel (sybren) created this revision.
Harbormaster completed remote builds in B13284: Diff 34820.Mar 5 2021, 6:01 PM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)Mar 5 2021, 6:03 PM
Dalai Felinto (dfelinto) requested changes to this revision.EditedMar 5 2021, 6:44 PM
Dalai Felinto (dfelinto) added a subscriber: Dalai Felinto (dfelinto).

The internal folder is not getting the Blender version correctly.

Running tar -Jtvf blender-2.93.0-alpha.tar.xz I get:
blender-$VERSION/release/datafiles/fonts/bmonofont-i18n.ttf.

Also the script is not deleting the blender-2.93.0-alpha-manifest.txt after creating the package. If this is intentional could you please mention in the patch description- since it is a different behaviour than what the bash script is doing at the moment.

build_files/utils/make_source_archive.py
108

This is not working. Did you forget to pass the $VERSION to the function?

This revision now requires changes to proceed.Mar 5 2021, 6:44 PM
Sybren A. Stüvel (sybren) updated this revision to Diff 34865.Mar 8 2021, 10:56 AM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)
  • Add little doctest for version-to-string conversion
  • Actually call the cleanup function
  • Ensure the tarball correctly contains the Blender version number
  • Store MD5 sum in same file as the shell script did (xxx.md5sum)
Harbormaster completed remote builds in B13314: Diff 34865.Mar 8 2021, 10:56 AM
Sybren A. Stüvel (sybren) added inline comments.Mar 8 2021, 10:57 AM
build_files/utils/make_source_archive.py
108

That's just a stupid copy-paste error.

Sybren A. Stüvel (sybren) marked an inline comment as done.Mar 8 2021, 10:57 AM
Dalai Felinto (dfelinto) added a comment.Mar 8 2021, 12:11 PM

Plenty of fixes, thanks Sybren!
There is a remaining issue though: the md5sum is now storing the full path of the tar.xz file, while the bash had only the package name:

Patch: e9f4f3c8ea0ceee6c9f3d5875d17ad4c /home/dfelinto/src/blender/blender/blender-2.93.0.tar.xz.
Master: e9f4f3c8ea0ceee6c9f3d5875d17ad4c blender-2.93.0.tar.xz.

This was already present in your original patch, I just didn't notice then.

Sybren A. Stüvel (sybren) updated this revision to Diff 34879.EditedMar 8 2021, 1:38 PM
  • Use base name of tarball in MD5sum file.

Thanks for the thorough review @Dalai Felinto (dfelinto) , it shows how silly it was of me to try and do this quickly on a Friday afternoon.

Harbormaster completed remote builds in B13326: Diff 34879.Mar 8 2021, 1:38 PM
Dalai Felinto (dfelinto) accepted this revision.Mar 8 2021, 3:34 PM
Dalai Felinto (dfelinto) added 1 blocking reviewer(s): Campbell Barton (campbellbarton).
Campbell Barton (campbellbarton) requested changes to this revision.Mar 8 2021, 4:02 PM

Looks good.

Requesting minor changes, no need for extra review iteration once resolved.

build_files/utils/make_source_archive.py
2

This script isn't going to work on windows (expecting tar & git commands to exist for example), also assuming forward slashes.

Since this is no longer a shell script, it's not obvious this is Unix only, there should be a warning/error if it runs on windows.

69

It's good specify encoding=utf-8 when opening files to avoid encoding issues. Same for writing the the manifest.

This revision now requires changes to proceed.Mar 8 2021, 4:02 PM
Sybren A. Stüvel (sybren) marked an inline comment as done.Mar 8 2021, 5:05 PM
Sybren A. Stüvel (sybren) added inline comments.
build_files/utils/make_source_archive.py
2

This script isn't going to work on windows (expecting tar & git commands to exist for example)

There can be tar & git commands on Windows too.

also assuming forward slashes.

There is no such assumption. The only place in the code where there are forward slashes is line 63:

version_path = blender_srcdir / "source/blender/blenkernel/BKE_blender_version.h"

but that's not string concatenation, that's using pathlib.Path, which will translate the string to the appropriate pathlib.Path subclass.

69

It's good specify encoding=utf-8 when opening files to avoid encoding issues. Same for writing the the manifest.

Sybren A. Stüvel (sybren) updated this revision to Diff 34891.Mar 8 2021, 5:07 PM
Sybren A. Stüvel (sybren) marked an inline comment as done.
  • Explicit encoding
Harbormaster completed remote builds in B13330: Diff 34891.Mar 8 2021, 5:07 PM
Sybren A. Stüvel (sybren) added a comment.Mar 8 2021, 5:08 PM
In D10629#269354, @Campbell Barton (campbellbarton) wrote:

no need for extra review iteration once resolved.

Then why mark as "changes requested"? If you trust me to land the patch correctly, just accept it. Now I'd have to land a not-accepted patch.

build_files/utils/make_source_archive.py
69

Ignore that comment, it shouldn't have been submitted.

Sybren A. Stüvel (sybren) updated this revision to Diff 34892.Mar 8 2021, 5:12 PM
  • Remove old shell script
Harbormaster completed remote builds in B13331: Diff 34892.Mar 8 2021, 5:12 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 8 2021, 6:10 PM
Closed by commit rB8771f015f50e: Python version of `make_source_archive.sh` (authored by Sybren A. Stüvel (sybren)). · Explain Why
This revision was automatically updated to reflect the committed changes.
Sybren A. Stüvel (sybren) added a commit: rB8771f015f50e: Python version of `make_source_archive.sh`.
Campbell Barton (campbellbarton) added a comment.EditedMar 9 2021, 12:53 AM

Reply inline, I wasn't sure if git outputs forward slashes or not on windows.

build_files/utils/make_source_archive.py
2

Forward slashes are assumed if skip_addon_contrib and submodule == "release/scripts/addons_contrib":, also, while possible, I didn't think having utilities like md5sum, tar & git all in the PATH. Isn't so common on Windows.

Revision Contents
Changeset List

PathSize
2 lines
build_files/
utils/
191 lines

Diff 34891

View Options

GNUmakefile

Loading...
View Options

build_files/utils/make_source_archive.py

Loading...