[upstream_utils] Rework upstream_utils scripts #6829

KangarooKoala · 2024-07-13T23:07:31Z

Notes:

For some reason, when running the script locally, mpack would get changed, and I'm not sure why. If I ran it on main, I could also have some local changes, but it'd be a different set of local changes. I'll wait to see what happens on CI.
Eigen and sleipnir had changes to the index lines of the patches when running locally, and I'm not sure why.
Stack walker also got trailing whitespace when running locally, for some reason.
While working on this I had some ideas for additional commands. I don't know if we'd want to implement those in this PR or in separate PR(s).
- ./<lib>.py info: Displays information about the library. (Mostly useful for clone directory, URL, tag, and patch list, but we might as well include the rest of the constructor arguments)
- ./<lib>.py reset: Effectively ./<lib>.py rebase <current tag>- Resets the clone to the tag and applies the patches. (Useful for adding new patches)
- Make the new-tag argument to format-patch default to the current tag. (Useful for adding new patches)
- Warnings about updating the tag when there's an explanatory comment above it. (e.g., eigen.py)
- Warnings about mismatches between the supplied patch list and patches in the patch directory.

Turns out that is how wpiformat wants it

calcmogul · 2024-07-14T22:50:16Z

While working on this I had some ideas for additional commands. I don't know if we'd want to implement those in this PR or in separate PR(s).

Implementing them here seems fine.

calcmogul

I tried updating Sleipnir, and it broke:

[tav@myriad upstream_utils]$ ./sleipnir.py clone
INFO: Opening repository at /tmp/Sleipnir
Cloning into 'Sleipnir'...
remote: Enumerating objects: 709, done.
remote: Counting objects: 100% (5/5), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 709 (delta 0), reused 5 (delta 0), pack-reused 704
Receiving objects: 100% (709/709), 500.16 KiB | 1.09 MiB/s, done.
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (16/16), done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 56 (delta 0), reused 5 (delta 0), pack-reused 40
Receiving objects: 100% (56/56), 9.78 KiB | 9.78 MiB/s, done.
remote: Enumerating objects: 206, done.
remote: Counting objects: 100% (127/127), done.
remote: Compressing objects: 100% (120/120), done.
remote: Total 206 (delta 28), reused 10 (delta 7), pack-reused 79
Receiving objects: 100% (206/206), 449.69 KiB | 2.32 MiB/s, done.
Resolving deltas: 100% (34/34), done.
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (14/14), done.
remote: Compressing objects: 100% (13/13), done.
remote: Total 56 (delta 0), reused 8 (delta 0), pack-reused 42
Receiving objects: 100% (56/56), 9.75 KiB | 1.39 MiB/s, done.
remote: Enumerating objects: 27, done.
remote: Counting objects: 100% (20/20), done.
remote: Compressing objects: 100% (20/20), done.
remote: Total 27 (delta 3), reused 2 (delta 0), pack-reused 7
Receiving objects: 100% (27/27), 25.70 KiB | 548.00 KiB/s, done.
Resolving deltas: 100% (3/3), done.
HEAD is now at b6ffa2d Don't use pool allocator on Windows (#597)
[tav@myriad upstream_utils]$ ./sleipnir.py rebase 9b50f5d293d74f37f95788ea6cc25a87c32c3d7c
INFO: Opening repository at /tmp/Sleipnir
From https://github.com/SleipnirGroup/Sleipnir
 * branch            9b50f5d293d74f37f95788ea6cc25a87c32c3d7c -> FETCH_HEAD
HEAD is now at b6ffa2d Don't use pool allocator on Windows (#597)
Successfully rebased and updated detached HEAD.
[tav@myriad upstream_utils]$ ./sleipnir.py format-patch
INFO: Opening repository at /tmp/Sleipnir
0001-Migrate-from-pybind11-to-nanobind-598.patch
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (16/16), done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 56 (delta 0), reused 6 (delta 0), pack-reused 40
Receiving objects: 100% (56/56), 9.78 KiB | 9.78 MiB/s, done.
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 1 (delta 0), pack-reused 0
Receiving objects: 100% (2/2), 4.64 KiB | 4.64 MiB/s, done.
0002-Only-use-clang-cl-in-Windows-Python-builds-599.patch
0003-Remove-using-enum-declarations.patch
0004-Use-fmtlib.patch
0005-Remove-unsupported-constexpr.patch
0006-Use-wpi-SmallVector.patch
0007-Suppress-clang-tidy-false-positives.patch
Traceback (most recent call last):
  File "/home/tav/frc/wpilib/allwpilib/upstream_utils/./sleipnir.py", line 74, in <module>
    main()
  File "/home/tav/frc/wpilib/allwpilib/upstream_utils/./sleipnir.py", line 70, in main
    sleipnir.main()
  File "/home/tav/frc/wpilib/allwpilib/upstream_utils/upstream_utils.py", line 466, in main
    self.format_patch(args.new_tag)
  File "/home/tav/frc/wpilib/allwpilib/upstream_utils/upstream_utils.py", line 399, in format_patch
    os.rename(f, os.path.join(patch_dest, f))
OSError: [Errno 18] Invalid cross-device link: '0001-Migrate-from-pybind11-to-nanobind-598.patch' -> '/home/tav/frc/wpilib/allwpilib/upstream_utils/sleipnir_patches/0001-Migrate-from-pybind11-to-nanobind-598.patch'

upstream_utils.py should delete the contents of the patch folder first, then use shutil.move() on the new patches.

Clear the patch directory first Use shutil.move instead to handle different filesystems for temp dir and allwpilib

calcmogul · 2024-07-16T17:04:49Z

upstream_utils generated two more patches than it should have for the Sleipnir update. One of these commands should be updating the tag in sleipnir.py so it generates patches for the correct range.

[tav@myriad upstream_utils]$ ./sleipnir.py clone
INFO: Opening repository at /tmp/Sleipnir
Cloning into 'Sleipnir'...
remote: Enumerating objects: 709, done.
remote: Counting objects: 100% (5/5), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 709 (delta 0), reused 5 (delta 0), pack-reused 704
Receiving objects: 100% (709/709), 500.16 KiB | 1.83 MiB/s, done.
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (16/16), done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 56 (delta 0), reused 5 (delta 0), pack-reused 40
Receiving objects: 100% (56/56), 9.78 KiB | 9.78 MiB/s, done.
remote: Enumerating objects: 206, done.
remote: Counting objects: 100% (127/127), done.
remote: Compressing objects: 100% (120/120), done.
remote: Total 206 (delta 28), reused 10 (delta 7), pack-reused 79
Receiving objects: 100% (206/206), 449.69 KiB | 2.71 MiB/s, done.
Resolving deltas: 100% (34/34), done.
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (14/14), done.
remote: Compressing objects: 100% (13/13), done.
remote: Total 56 (delta 0), reused 8 (delta 0), pack-reused 42
Receiving objects: 100% (56/56), 9.75 KiB | 9.75 MiB/s, done.
remote: Enumerating objects: 27, done.
remote: Counting objects: 100% (20/20), done.
remote: Compressing objects: 100% (20/20), done.
remote: Total 27 (delta 3), reused 2 (delta 0), pack-reused 7
Receiving objects: 100% (27/27), 25.70 KiB | 560.00 KiB/s, done.
Resolving deltas: 100% (3/3), done.
HEAD is now at b6ffa2d Don't use pool allocator on Windows (#597)
[tav@myriad upstream_utils]$ ./sleipnir.py rebase 9b50f5d293d74f37f95788ea6cc25a87c32c3d7c
INFO: Opening repository at /tmp/Sleipnir
From https://github.com/SleipnirGroup/Sleipnir
 * branch            9b50f5d293d74f37f95788ea6cc25a87c32c3d7c -> FETCH_HEAD
HEAD is now at b6ffa2d Don't use pool allocator on Windows (#597)
Successfully rebased and updated detached HEAD.
[tav@myriad upstream_utils]$ ./sleipnir.py format-patch
INFO: Opening repository at /tmp/Sleipnir
0001-Migrate-from-pybind11-to-nanobind-598.patch
remote: Enumerating objects: 56, done.
remote: Counting objects: 100% (16/16), done.
remote: Compressing objects: 100% (15/15), done.
Receiving objects: 100% (56/56), 9.78 KiB | 9.78 MiB/s, done.
remote: Total 56 (delta 0), reused 6 (delta 0), pack-reused 40
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 1 (delta 0), pack-reused 0
Receiving objects: 100% (2/2), 4.64 KiB | 4.64 MiB/s, done.
0002-Only-use-clang-cl-in-Windows-Python-builds-599.patch
0003-Remove-using-enum-declarations.patch
0004-Use-fmtlib.patch
0005-Remove-unsupported-constexpr.patch
0006-Use-wpi-SmallVector.patch
0007-Suppress-clang-tidy-false-positives.patch
WARNING: Automatically updating tag in line 59 with a comment above it that may need updating.
WARNING: The patch directory has patches ['0001-Migrate-from-pybind11-to-nanobind-598.patch', '0002-Only-use-clang-cl-in-Windows-Python-builds-599.patch', '0003-Remove-using-enum-declarations.patch', '0004-Use-fmtlib.patch', '0005-Remove-unsupported-constexpr.patch', '0006-Use-wpi-SmallVector.patch', '0007-Suppress-clang-tidy-false-positives.patch'] not in the patch list
WARNING: The patch list has patches ['0001-Remove-using-enum-declarations.patch', '0002-Use-fmtlib.patch', '0003-Remove-unsupported-constexpr.patch', '0004-Use-wpi-SmallVector.patch', '0005-Suppress-clang-tidy-false-positives.patch'] not in the patch directory

[tav@myriad allwpilib]$ git st
On branch upstream-utils-commands
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    upstream_utils/sleipnir_patches/0001-Remove-using-enum-declarations.patch
        deleted:    upstream_utils/sleipnir_patches/0002-Use-fmtlib.patch
        deleted:    upstream_utils/sleipnir_patches/0003-Remove-unsupported-constexpr.patch
        deleted:    upstream_utils/sleipnir_patches/0004-Use-wpi-SmallVector.patch
        deleted:    upstream_utils/sleipnir_patches/0005-Suppress-clang-tidy-false-positives.patch

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        upstream_utils/sleipnir_patches/0001-Migrate-from-pybind11-to-nanobind-598.patch
        upstream_utils/sleipnir_patches/0002-Only-use-clang-cl-in-Windows-Python-builds-599.patch
        upstream_utils/sleipnir_patches/0003-Remove-using-enum-declarations.patch
        upstream_utils/sleipnir_patches/0004-Use-fmtlib.patch
        upstream_utils/sleipnir_patches/0005-Remove-unsupported-constexpr.patch
        upstream_utils/sleipnir_patches/0006-Use-wpi-SmallVector.patch
        upstream_utils/sleipnir_patches/0007-Suppress-clang-tidy-false-positives.patch

no changes added to commit (use "git add" and/or "git commit -a")

KangarooKoala · 2024-07-16T17:09:51Z

Seems like rebase should update the tag, even though it wasn't specified in the original issue. When in the rebase process should we update the tag? (This is important since applying the patches and rebasing could potentially cause the code to crash) Also, do we still want to format-patch to update the tag?

calcmogul · 2024-07-16T17:10:38Z

If format-patch was supposed to update the tag, it didn't in my testing. Are you supposed to specify the new tag for both rebase and format-patch? If so, we should probably require it to avoid users like me shooting themselves in the foot. :)

KangarooKoala · 2024-07-16T17:20:55Z

Currently, format-patch with no args uses the tag in the script, which is not updated by rebase. (The original proposed workflow for updating was to specify the new tag in both rebase and format-patch, and when I added the default value to the format-patch tag, I was thinking about adding new patches, not rebasing, so I missed that format-patch with no args after a rebase would appear to make sense.)

Going forward, I can think of two options:

rebase updates the tag in the script. This means that clone; rebase <tag>; format-patch; copy-upstream-to-thirdparty will work as expected.
Document that format-patch without arguments should not be called after rebase, possibly adding some logic to detect if that's the case and warning if so.

I like 1 better since when possible, it usually goes better to make stuff work than to tell people not to do it.

calcmogul · 2024-07-16T17:23:31Z

Having rebase override the old script tag puts the tag and patches in the upstream_utils folder in an inconsistent state. We could avoid that by putting all metadata in the upstream git repo.

One thing I can think of is having a movable tag in the upstream repo that designates the root of the patch series (e.g., upstream_utils_root). clone would add upstream_utils_root at the old tag, rebase would bump upstream_utils_root after applying the patches but before performing the rebase, and format-patch would use upstream_utils_root to generate the proper patchset and update the script tag. We could include the tag name or commit hash as a suffix on the upstream_utils_root tag name so format-patch knows what to put, as long as we're diligent about deleting the old name first in rebase.

calcmogul

The Sleipnir update works now.

upstream_utils.py's functions need docstrings.

upstream_utils/upstream_utils.py

upstream_utils/README.md

Don't redundantly specify the tag

KangarooKoala added 3 commits July 13, 2024 15:49

Rework upstream_utils scripts

00d91d2

Convert single quote strings into double quote strings

a1def44

Fix upstream_utils README

a5bf968

KangarooKoala requested a review from a team as a code owner July 13, 2024 23:07

Revert changes to single quote strings

47f7a75

Turns out that is how wpiformat wants it

KangarooKoala added 4 commits July 14, 2024 17:56

Add some conveniences

d25c653

Update upstream_utils README

164366e

Fix bug with autoupdating the stack_walker tag

016ecaf

Pass patch_list to mpack constructor

71b9f07

calcmogul requested changes Jul 15, 2024

View reviewed changes

KangarooKoala added 2 commits July 15, 2024 22:36

Fix format-patch

53e90d0

Clear the patch directory first Use shutil.move instead to handle different filesystems for temp dir and allwpilib

Merge branch 'main' into upstream-utils-commands

4e15117

Add root tag

2c23924

calcmogul requested changes Jul 16, 2024

View reviewed changes

KangarooKoala added 3 commits July 16, 2024 13:27

Set the root tag in reset() as well

1e17563

Add docstrings

7bf00dd

Break up long lines

7792a0c

calcmogul reviewed Jul 16, 2024

View reviewed changes

upstream_utils/upstream_utils.py Outdated Show resolved Hide resolved

Set root tag after applying patches in reset()

5eb7a57

calcmogul requested changes Jul 16, 2024

View reviewed changes

upstream_utils/README.md Outdated Show resolved Hide resolved

KangarooKoala added 2 commits July 16, 2024 16:45

Correct upstream_utils README

ba72287

Don't redundantly specify the tag

Remove format-patch tag argument

516222e

calcmogul approved these changes Jul 16, 2024

View reviewed changes

PeterJohnson merged commit 5f261a8 into wpilibsuite:main Jul 17, 2024
33 checks passed

KangarooKoala deleted the upstream-utils-commands branch July 17, 2024 00:20

calcmogul mentioned this pull request Jul 17, 2024

[ci] Add /update command to update upstream_utils #6817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[upstream_utils] Rework upstream_utils scripts #6829

[upstream_utils] Rework upstream_utils scripts #6829

KangarooKoala commented Jul 13, 2024

calcmogul commented Jul 14, 2024

calcmogul left a comment •

edited

Loading

calcmogul commented Jul 16, 2024 •

edited

Loading

KangarooKoala commented Jul 16, 2024

calcmogul commented Jul 16, 2024 •

edited

Loading

KangarooKoala commented Jul 16, 2024

calcmogul commented Jul 16, 2024 •

edited

Loading

calcmogul left a comment

[upstream_utils] Rework upstream_utils scripts #6829

[upstream_utils] Rework upstream_utils scripts #6829

Conversation

KangarooKoala commented Jul 13, 2024

calcmogul commented Jul 14, 2024

calcmogul left a comment • edited Loading

Choose a reason for hiding this comment

calcmogul commented Jul 16, 2024 • edited Loading

KangarooKoala commented Jul 16, 2024

calcmogul commented Jul 16, 2024 • edited Loading

KangarooKoala commented Jul 16, 2024

calcmogul commented Jul 16, 2024 • edited Loading

calcmogul left a comment

Choose a reason for hiding this comment

calcmogul left a comment •

edited

Loading

calcmogul commented Jul 16, 2024 •

edited

Loading

calcmogul commented Jul 16, 2024 •

edited

Loading

calcmogul commented Jul 16, 2024 •

edited

Loading