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.

Sign up for GitHub

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

Jump to bottom

Fix "crane layout gc" failing on Windows wit "Error: cannot parse hash: "sha256\\0401ae60059.." (#1942) #1944

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix "crane layout gc" failing on Windows wit "Error: cannot parse hash: "sha256\\0401ae60059.." (#1942) #1944

wants to merge 1 commit into from

Conversation

bartoszpop
Copy link

@bartoszpop bartoszpop commented May 8, 2024 •
edited
Loading

This pull request updates "layout gc" to use os-specific path separator instead of "/" (fixes #1942).

@bartoszpop
Copy link
Author

bartoszpop commented May 19, 2024 •
edited
Loading

@imjasonh @thesayyn @jonjohnsonjr would you have some time to review? it is blocking us from migrating to Bazel

martianoff
martianoff approved these changes Jun 5, 2024
View reviewed changes
Copy link

@martianoff martianoff left a comment

Choose a reason for hiding this comment

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

nice catch

@bartoszpop
Copy link
Author

@imjasonh any chance to have it merged?

thesayyn
thesayyn approved these changes Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

Can you add some tests for this?

@thesayyn
Copy link
Collaborator

it is blocking us from migrating to Bazel

If you need this for rules_oci, i might interest you in rules_oci 2.x which probably wouldn't have this problem.

@martianoff
Copy link

rules_oci 2.x

i've tried but it doesn't work on windows due to other issues

@bartoszpop
Copy link
Author

@thesayyn the unit test is already there. gc_test.go#TestGcIndex fails on Windows without the above fix:

--- FAIL: TestGcIndex (0.00s)
gc_test.go:39: GarbageCollect() = cannot parse hash: "sha256\05f95b26ed10668b7183c1e2da98610e91372fa9f510046d4ce5812addad86b5"

Also, os.PathSeparator is a constant hence it cannot be modified at runtime for unit test purposes.

@bartoszpop
Copy link
Author

@imjasonh @thesayyn @jonjohnsonjr can you please merge this fix? When is the next release planned?

@bartoszpop
Copy link
Author

@mattmoor is there a chance you could merge this pr and release?

@bartoszpop bartoszpop requested a review from thesayyn July 31, 2024 00:07
@github-actions GitHub Actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thesayyn thesayyn thesayyn approved these changes

@martianoff martianoff martianoff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@bartoszpop @thesayyn @martianoff