Init: chromium darwin #244152

michaelCTS · 2023-07-18T11:39:33Z

Description of changes

Provides a simple chromium package for darwin as well as a makeDarwinApp.

There are still some possible improvements, but I would like to keep this PR small and add those improvements in later PRs instead of having one enormous PR.

an update script
a common mkDerivation.meta (though that may be messy)
makeDarwinApp should be able to retrieve the version from the source (Info.plist should contain a field with the app version)

Notes

The diff looks like chromium/default.nix was edited, but I moved it all into chromium/linux and created a new one. The diff is unfortunately not better, but I hope that won't be a problem

Things done

closes #247855

toonn

Would be nice if we could get a Linux maintainer's opinion on the reorganizing of the package.

pkgs/build-support/make-darwin-app/default.nix

pkgs/applications/networking/browsers/chromium/darwin/default.nix

ghost · 2023-07-18T23:55:52Z

Would be nice if we could get a Linux maintainer's opinion on the reorganizing of the package.

Yeah that reorg is going to create a massive merge conflict with pending PRs like #229265.

ghost

Since chromium is built from source and this isn't, they zero Nix code; there's really no benefit to having them be the same package. @toon had the right idea; rename this to chromium-bin and put an if stdenv.hostPlatform.isDarwin then chromium-bin else ... in pkgs/top-level/all-packages.nix.

pkgs/build-support/make-darwin-app/default.nix

michaelCTS · 2023-07-19T07:17:41Z

@amjoseph-nixpkgs there doesn't seem to be an existing chromium-bin. There wouldn't be a need for if stdenv.hostPlatform.isDarwin then chromium-bin else ... in all-packages.nix, correct?

ghost · 2023-07-19T07:57:03Z

@amjoseph-nixpkgs there doesn't seem to be an existing chromium-bin.

Right; I was suggesting that you create one. But, as you point out:

There wouldn't be a need for if stdenv.hostPlatform.isDarwin then chromium-bin else ... in all-packages.nix, correct?

I guess if you really want to avoid creating another top-level entry you could just have callPackage ../applications/networking/browsers/chromium/darwin.nix {}.

The -bin convention is pretty well-established, though. We have 163 packages in all-packages.nix with the -bin suffix (including firefox).

Changes implemented. Thanks!

ghost · 2023-07-19T19:52:43Z

Hey, looking much better, thanks a bunch for reorganizing this. I'll leave the rest of the review up to people who have Darwin hardware to test on. Nice work!

It's not clean, but it works. What's missing is an update script and how to get the actual version without downloading the entire archive first. extractDarwinApp is a helper for making derivations from .app and .dmg bundles.

toonn

Much simpler this way, I think making a new package was for the best.

pkgs/applications/networking/browsers/chromium-bin/default.nix

pkgs/build-support/extract-darwin-app/default.nix

toonn · 2023-07-22T22:10:56Z

pkgs/build-support/extract-darwin-app/default.nix

+
+    mkdir -p $out/bin
+    if [[ "${toString isDmg}" = "1" ]] ; then
+      # .dmg uses sourceRoot, which means PWD is already in the sourceRoot


Can't we avoid setting the sourceRoot to inside the .app bundle so that we can do away with these branches and just copy the bundle in both cases?

There are multiple usecases.

the source is unpacked to contain a folder with a single subfolder ${appName}.app/

That means we can copy the subfolder with no processing

the source is unpacked to contain a folder with multiple subfolders of which one is the .app folder

This makes the default unpackPhase unhappy as it expects only a single folder https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L1136
It is that error which added the necessity for sourcesRoot.

the source is a folder

I'm now noticing that this isn't supported by the function in its current form.

But this can be the most difficult or the easiest, depending on how it's handled. The reason being at some point the there's a cd into the folder by mkDerivation --> a ${appName}.app folder will reveal Contents/ when a ls is run in the installPhase.

This means, either similar processing to now which still requires branching, or detecting whether it's a valid bundle and then doing processing like is done now.

If you have a suggestion to handle this without branching, that'd be great.

2f85c6e treats all the usecases above but with the existing branches. I couldn't conceive a solution without branches.

Just setting sourceRoot = "." should work. It only makes passing an app as the source slightly harder but I don't think that'd be a common case?

It does seem like a valid usecase. Is it a problem that it's handled?

It makes everything more complex for very little gain though. When would you want to use a straight app as source and be unable to use the directory containing it instead?

@michaelCTS, do you disagree strongly with the simplification?

Yes, I do. It's not clear to me why we would remove support for a usecase just because of a single if/else, and wait until/if someone does stumble upon the usecase to then have another discussion about a conditional block.

Edit: I currently don't have the bandwidth to make modifications anyway, so it's OK with me if you edit it or even fork it and make another PR. You should have edit access now

The arguments and documentation implied that only `.app` and `.dmg` we supported, but that wasn't true. The case for `src = ./some/folder` and non-dmg archives with multiple folders was not supported. Now, the user can specify srcHasManySubfolders which sets the `sourceRoot` for `mkDerivation` to a default value. The user also can directly set `sourceRoot` themselves.

This allows nix to handle the supported platforms instead of throwing a string for unsupported platforms.

jakuzure · 2023-09-24T13:03:40Z

Hey, looking much better, thanks a bunch for reorganizing this. I'll leave the rest of the review up to people who have Darwin hardware to test on. Nice work!

I just tried it out via home-manager (set the package option to chromium-bin) and it seems to be working fine. I couldn't get the extensions option to work though, but that might be related to the bin maybe? Installing them directly in the browser worked fine

michaelCTS · 2023-09-26T12:22:16Z

@jakuzure thanks for testing. Not sure what's up with the extensions in home-manager and how it's implemented. A related issue would have to be created there if this is merged.

WeetHet · 2024-07-03T16:11:43Z

Hi, any updates on when this could be merged and if I can help it to happen sooner somehow?

michaelCTS · 2024-07-04T07:36:19Z

@WeetHet as per my last comment, there's no time for this on my end. I'm actually going to sell my mac as it's not necessary anymore.

Feel free to branch off my branch and make the changes @toonn wanted (that I disagreed with). It's probably what will get the PR approved.

lrworth · 2024-07-05T01:15:33Z

I've opened #324701, simplifying this as much as possible in order to get it merged. Someone with more motivation may get something like extract-darwin-app working in the future.

michaelCTS self-assigned this Jul 18, 2023

michaelCTS added the 6.topic: darwin Running or building packages on Darwin label Jul 18, 2023

michaelCTS force-pushed the chromium-darwin branch 2 times, most recently from a960814 to 80633f7 Compare July 18, 2023 11:50

michaelCTS requested review from soupglasses, primeos and a user July 18, 2023 11:59

michaelCTS force-pushed the chromium-darwin branch 3 times, most recently from f479f21 to 1ea3326 Compare July 18, 2023 14:57

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 18, 2023

toonn reviewed Jul 18, 2023

View reviewed changes

ghost previously requested changes Jul 19, 2023

View reviewed changes

pkgs/build-support/make-darwin-app/default.nix Outdated Show resolved Hide resolved

michaelCTS force-pushed the chromium-darwin branch 3 times, most recently from 0ef984b to 53ce772 Compare July 19, 2023 17:47

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Jul 20, 2023

Init chromium-bin aarch64-darwin + extractDarwinApp

b33f2c6

It's not clean, but it works. What's missing is an update script and how to get the actual version without downloading the entire archive first. extractDarwinApp is a helper for making derivations from .app and .dmg bundles.

michaelCTS force-pushed the chromium-darwin branch from 53ce772 to b33f2c6 Compare July 21, 2023 09:23

toonn reviewed Jul 22, 2023

View reviewed changes

Michael Vogel added 2 commits July 25, 2023 17:32

refactor(chromium-bin) Only return darwin package

5f5e960

This allows nix to handle the supported platforms instead of throwing a string for unsupported platforms.

1sixth mentioned this pull request Aug 8, 2023

Package request: chromium darwin support #247855

Open

CMCDragonkai mentioned this pull request Aug 30, 2023

Does this still work? ariutta/nix-bundle-macos#1

Open

toonn mentioned this pull request Sep 14, 2023

gitx-bin: init at 0.23 #225130

Draft

12 tasks

emilylange mentioned this pull request Jan 28, 2024

Package request: ungoogled-chromium for macOS #283884

Open

emilylange mentioned this pull request Feb 7, 2024

thorium-browser: init at 120.0.6099.235 #284085

Merged

13 tasks

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 3, 2024

michaelCTS closed this Jul 4, 2024

lrworth mentioned this pull request Jul 5, 2024

Init: chromium-bin #324701

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init: chromium darwin #244152

Init: chromium darwin #244152

michaelCTS commented Jul 18, 2023 •

edited by kalbasit

Loading

toonn left a comment

ghost commented Jul 18, 2023

ghost left a comment

michaelCTS commented Jul 19, 2023

ghost commented Jul 19, 2023

ghost commented Jul 19, 2023

toonn left a comment

toonn Jul 22, 2023

michaelCTS Jul 25, 2023

michaelCTS Jul 25, 2023

toonn Jul 25, 2023

michaelCTS Jul 26, 2023

toonn Jul 26, 2023

toonn Sep 14, 2023

michaelCTS Sep 15, 2023 •

edited

Loading

jakuzure commented Sep 24, 2023

michaelCTS commented Sep 26, 2023

WeetHet commented Jul 3, 2024

michaelCTS commented Jul 4, 2024

lrworth commented Jul 5, 2024

Init: chromium darwin #244152

Init: chromium darwin #244152

Conversation

michaelCTS commented Jul 18, 2023 • edited by kalbasit Loading

Description of changes

Things done

toonn left a comment

Choose a reason for hiding this comment

ghost commented Jul 18, 2023

ghost left a comment

Choose a reason for hiding this comment

michaelCTS commented Jul 19, 2023

ghost commented Jul 19, 2023

ghost commented Jul 19, 2023

toonn left a comment

Choose a reason for hiding this comment

toonn Jul 22, 2023

Choose a reason for hiding this comment

michaelCTS Jul 25, 2023

Choose a reason for hiding this comment

michaelCTS Jul 25, 2023

Choose a reason for hiding this comment

toonn Jul 25, 2023

Choose a reason for hiding this comment

michaelCTS Jul 26, 2023

Choose a reason for hiding this comment

toonn Jul 26, 2023

Choose a reason for hiding this comment

toonn Sep 14, 2023

Choose a reason for hiding this comment

michaelCTS Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

jakuzure commented Sep 24, 2023

michaelCTS commented Sep 26, 2023

WeetHet commented Jul 3, 2024

michaelCTS commented Jul 4, 2024

lrworth commented Jul 5, 2024

michaelCTS commented Jul 18, 2023 •

edited by kalbasit

Loading

michaelCTS Sep 15, 2023 •

edited

Loading