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

installer: don't assume GNU diff #7948

Merged
merged 1 commit into from
May 25, 2023
Merged

Conversation

mkenigs
Copy link
Contributor

@mkenigs mkenigs commented Mar 3, 2023

macOS Ventura ships with it's own version of diff. Try to output a similar diff with Apple diff as with GNU diff, instead of failing

Helps #7286

@mkenigs mkenigs requested a review from edolstra as a code owner March 3, 2023 00:31
@mkenigs
Copy link
Contributor Author

mkenigs commented Mar 3, 2023

@abathur

@abathur
Copy link
Member

abathur commented Mar 3, 2023

Any chance you can attach a screenshot of the format it produces for reference?

There are instructions for getting the installer tests working in https://github.com/NixOS/nix/blob/master/doc/manual/src/contributing/hacking.md#installer-tests.

They are of, eh, marginal utility here. They help demonstrate that the happy path works w/o a human having to do it.

They also technically generate installers a human can use, but since gha doesn't have arm runners for macos, we'd have to hunt down someone in the venn diagram of Intel + Ventura to see if they are open to engineering a test that would trigger this path. Since you have directly tested that part of the code, I think it's fine to skip.

Lmk if that isn't tolerable for any reason and I can make a copy and push this to get a CI result.

Copy link
Member

@abathur abathur left a comment

Choose a reason for hiding this comment

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

This looks like a pretty clear win over the current fail-and-print-help that Ventura users can currently end up with.

I'd personally say that merging will close #7286

For reference, here's what the format looks like in GNU diff on macOS Catalina:

Screen Shot 2023-03-02 at 9 22 52 PM

@mkenigs
Copy link
Contributor Author

mkenigs commented Mar 13, 2023

Sorry this got buried. Here's a comparison of Ventura diff on the left and GNU diff on the right:
Screenshot 2023-03-13 at 4 03 49 PM

@mkenigs
Copy link
Contributor Author

mkenigs commented Mar 13, 2023

As for testing, I'm happy to do whatever you think is best. I've tested by copying and pasting into a shell script I can run directly, but I haven't actually tested it in the context of an installation

@mkenigs
Copy link
Contributor Author

mkenigs commented May 24, 2023

@thufschmitt would you be able to merge this?

@thufschmitt
Copy link
Member

@thufschmitt would you be able to merge this?

Actually not, the CI doesn't run for some reason, so I can't merge. Can you try re-triggering it by pushing something on the branch? git commit --amend --no-edit && git push --force-with-lease should do it

macOS Ventura ships with it's own version of diff. Try to output a
similar diff with Apple diff as with GNU diff, instead of failing

Helps NixOS#7286
@mkenigs
Copy link
Contributor Author

mkenigs commented May 25, 2023

Thanks for looking at this! Pushed

@thufschmitt thufschmitt merged commit f41dd2c into NixOS:master May 25, 2023
@mkenigs mkenigs deleted the ventura-diff branch May 25, 2023 16:17
@fricklerhandwerk fricklerhandwerk added installer macos Nix on macOS, aka OS X, aka darwin labels Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installer macos Nix on macOS, aka OS X, aka darwin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants