Skip to content

Add --asset-dir to blame-copy-royal #2065

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

Merged
merged 2 commits into from
Jun 27, 2025

Conversation

cruessler
Copy link
Contributor

This is a follow-up to #2041. Without this change, blame-copy-royal can’t be used for files whose relative path starts with assets/ as this is also a path used by blame-copy-royal. This PR adds a parameter --asset-dir that allows overwriting the asset directory so that it can be set to an arbitrary value in case there is a conflict.

A similar option could also be provided for the name of the script itself.

@EliahKagan
Copy link
Member

EliahKagan commented Jun 26, 2025

CI runs on this upstream GitoxideLabs/gitoxide repository now use an image for windows-latest that contains Git 2.50.0 rather than Git 2.49.0, at least some of the time. That's responsible for the new CI failure here, which is completely unrelated to the code changes here, though I cannot say with certainty that these errors couldn't mask a relevant failure.

The new failing test cases are due to the fixture script error, occurring for each test that uses the affected script, that I described in the second half of #2053 (comment) ("The new failures with Git 2.50.0"). It happens due to Git 2.50.0 no longer supporting unrecognized -t operands for hash-object. These errors only happen with GIX_TEST_IGNORE_ARCHIVES=1. See discussion in #2053.

Since then, I've reproduced the errors locally on an x86_64 Arch Linux system. The results can be seen in this gist. That they don't happen on Ubuntu in the full test job yet is merely because it is still getting an ubuntu-latest image with Git 2.49.0. Once that has 2.50.0, which can also be expected to happen soon, we should expect it to happen there, too.

Interestingly, I also had a small number of other test failures, which appear to be entirely unrelated to these, on Arch Linux x86_64 with Git 2.50.0. Two of them happen without GIX_TEST_IGNORE_ARCHIVES=1. These seem to be the same pack-related test failures I had previously, and for some time, observed on s390x but no other architectures, down to the exact disagreeing hash values. That this would now happen, and in seemingly the same way, on x86_64 is something I find quite unexpected, but hopefully it will also lead to figuring out what is going on with it. These new failures also appear deterministic, and they are not among the failures discussed in #2053. I expect these pack-related failures most likely to start happening here in test-fast on ubuntu-latest, too. See that same gist for some other details and full test output, and #1890 for background.

Although I plan to try to fix this--and, at least with respect to the hash-object -t errors, I expect to be able to fix it--I don't know if I will manage to do anything on it today, nor when I will manage to do it. I'm about to be away from my computer for a number of hours, and I am unlikely to be able to begin working on this even for some time after I return. Therefore, I am posting this to share what I have found so far. (If I have time, I'll try at least to post a proper issue.)

@cruessler
Copy link
Contributor Author

@EliahKagan That’s helpful, thanks a lot for the context! For this PR, there’s no pressure to get it merged as it is related to it only. I’ll keep an eye on this topic, though, and will update this PR once main is ready. Thanks again!

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the update, and sorry for not catching this issue during the review.

@Byron Byron enabled auto-merge June 27, 2025 03:53
@Byron Byron force-pushed the add-asset-dir-to-blame-copy-royal branch from 30d4d6a to e616e87 Compare June 27, 2025 04:01
@EliahKagan
Copy link
Member

EliahKagan commented Jun 27, 2025

The test-32bit jobs run in debian:stable-slim containers. I believe these don't ship with a preinstalled python or python3 command on any architecture. But we are seeing that such commands are at least absent on debian:stable-slim for arm32v7. So they fail on e616e87.

Although we should be able to do this without Python, for now the best thing to do might be to install a Python interpreter in those containers.

  • It should be possible to do this by adding a step using the setup-python action, which will run in the container.
  • Or it could be done by adding the python-is-python3 package (for equivalent python and python3 commands) or python3 package (for only a python3 command) to the prerequisites array.
  • If it's acceptable to use python3 as the command name, then the lighter-weight approach of adding the python3-minimal package (and no other Python-related packages) to the prerequisites array might be best.

Any such change to the workflow could be undone later, if and when the fixture script is eventually modified to use only tools that the test suite otherwise requires.

@Byron Byron force-pushed the add-asset-dir-to-blame-copy-royal branch 2 times, most recently from 9ae314c to 3838fcd Compare June 27, 2025 04:37
@Byron
Copy link
Member

Byron commented Jun 27, 2025

Thanks a lot for the hint! If the python3-minimal in prerequisites approach, this PR will merge soon.

I definitely invite you to do this without python3. The approach was chosen because I thought it's easier to deal with one dependency than with multiple ones + bash portability issues - little did I know😅.

@Byron Byron force-pushed the add-asset-dir-to-blame-copy-royal branch from 3838fcd to 73a30f8 Compare June 27, 2025 04:41
@Byron Byron merged commit 3f2be40 into GitoxideLabs:main Jun 27, 2025
23 checks passed
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.

3 participants