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

fix: patch expect tests on macos #11132

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

Conversation

gridbugs
Copy link
Collaborator

A recent update to macos's patch command broke some of our tests. The relevant changes to the command's behaviour are that it's stricter about line numbers in patch metadata, and it no longer deletes files, truncating them instead. This updates dune's patch expect tests to accomodate this behaviour.

A recent update to macos's patch command broke some of our tests. The
relevant changes to the command's behaviour are that it's stricter about
line numbers in patch metadata, and it no longer deletes files,
truncating them instead. This updates dune's patch expect tests to
accomodate this behaviour.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs gridbugs force-pushed the fix-patch-expect-tests-on-macos branch from d83725e to 5fcaec6 Compare November 19, 2024 05:59
@maiste maiste changed the title Fix patch expect tests on macos fix: patch expect tests on macos Nov 19, 2024
match (Unix.stat filename).st_kind with
| S_REG ->
(* Some implementations of patch (e.g. the patch that ships with macOS
15.1) do not delete files, and instread truncate them to 0 length. If
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
15.1) do not delete files, and instread truncate them to 0 length. If
15.1) do not delete files, and instead truncate them to 0 length. If

Copy link
Collaborator

@maiste maiste left a comment

Choose a reason for hiding this comment

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

Spot a typo but otherwise LGTM 👍

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation. Looks like what macOS 15.1 did is a step back, given these patch features have been common patch extensions.

I agree with the approach, just a small comment on how to make the fix a bit more efficient and nicer.

15.1) do not delete files, and instread truncate them to 0 length. If
the file still exists after applying the patch, assert that it is now
empty. *)
assert (String.length (Io.String_path.read_file filename) == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think instead of reading the file I think it would be better to look at the stat metadata and read st_size. We already stat above and reading the size feels more direct than checking the length of the contents we read.

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

Successfully merging this pull request may close these issues.

3 participants