feat: Mac .pkg installer #1312

mistydemeo · 2024-08-12T23:23:51Z

This adds support for building Mac .pkg installers. This current implementation is intentionally a bit simplistic: it produces one .pkg per package in the user's workspace, without any special branding applied. We currently only allow the app identifier and install location to be configured, but we can consider adding further customization in the future.

Although I added support for configuring this in init, I ran into a bit of trouble serializing that value since the new config is set up as a struct rather than two separate fields.

Here's sample config from one app:

[workspace.metadata.dist.mac-pkg-config]
identifier = "dev.axo.cargo-dist"
install-location = "/usr/local"

And here's the No Name / Sans nom installer it produces:

Gankra · 2024-08-22T15:41:32Z

cargo-dist/src/backend/installer/macpkg.rs

+        let build_dir_target = TempDir::new()?;
+        let build_dir = build_dir_target.path();
+        let bindir = build_dir.join("bin");
+        fs::create_dir_all(&bindir)?;
+        let libdir = build_dir.join("lib");
+        fs::create_dir_all(&libdir)?;


Please no raw io errors, I really want to remove that error variant from DistError, because it's awful to slam a user with that.

We have several places where we do tempdirs we should be reusing, and axoasset APIs for others. Also camino.

Promoted the tempdir thing to a public method; making me wonder if we just want this to be in axoasset.

Converted the rest into axoasset calls except the symlinks, which axoasset doesn't know about yet. I can add that though, feels in scope.

cargo-dist/src/backend/installer/macpkg.rs

Gankra · 2024-08-22T16:18:48Z

cargo-dist/src/config.rs

+    /// Configuration specific to Mac .pkg installers
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(default)]
+    pub mac_pkg_config: Option<MacPkgConfig>,


glossing over this as something we can freely rejig in config 1.0...

Yep, like I mentioned elsewhere this is something I wanted to check in with you about wrt handling before/after config 1.0.

cargo-dist/src/init.rs

Gankra · 2024-08-22T16:20:39Z

cargo-dist/src/init.rs

+                return Err(DistError::MacPkgBundleIdentifierMissing {});
+            }
+
+            let prompt = r#"Please enter the installation prefix this .pkg should use."#;


should probably specify the default, or we shouldn't even ask? Not sure how typical it is to overload this.

Overloading it is pretty common. Many people are going to want the default /usr/local, which is in the PATH but shared with other software. Other people are going to prefer, say, /opt/mypkg, which is out of the default PATH but a prefix fully owned by them, etc.

I was going for an "accept NULL, unwrap to /usr/local later" approach. But I guess that means the default could shift under the user, which maybe isn't ideal - I guess "inline a default if not specified" is probably better.

cargo-dist/src/init.rs

Gankra · 2024-08-22T16:23:20Z

cargo-dist/src/tasks.rs

+            let binaries = variant.binaries.clone();
+            let target = &variant.target;
+            if !target.contains("darwin") {
+                continue;


Normally we warn here, I think?

I was following the pattern from the MSI, which doesn't warn here.

Gankra · 2024-08-22T16:24:18Z

cargo-dist/src/tasks.rs

+            // Compute which package we're actually building, based on the binaries
+            let mut package_info: Option<(String, PackageIdx)> = None;
+            for &binary_idx in &binaries {
+                let binary = self.binary(binary_idx);
+                if let Some((existing_spec, _)) = &package_info {
+                    // we haven't set ourselves up to bundle multiple packages yet
+                    if existing_spec != &binary.pkg_spec {
+                        return Err(DistError::MultiPackage {


...wild why was this logic exclusive to msi

Oh right I think it's because I needed to enforce this for the sake of calling into cargo-wix, but I don't think the pkg stuff has that limitation?

Oh, so the goal wasn't to create a single MSI containing every package but rather just that cargo-wix can't understand what to do when given multiple packages? I think I misunderstood the purpose of this guard then.

Right, I see now why we've got a separate conflict here. We're fetching some package-level metadata to pass to the package build - specifically, the identifier field is package-level metadata. We don't have the package directly accessible to us - just on the binaries - and if we did end up with a conflict of which package they resolve to, I don't know if I could get a better answer than the MSI path does.

Gankra

(also we need to disable the init code temporarily to land this)

Gankra · 2024-08-28T15:33:46Z

cargo-dist/src/init.rs

+        let mut new_table = toml_edit::table();
+        if let Some(new_table) = new_table.as_table_mut() {
+            new_table.insert("identifier", toml_edit::value(&mac_pkg_config.identifier));
+            if let Some(location) = &mac_pkg_config.install_location {
+                new_table.insert("install-location", toml_edit::value(location));
+            }
+            new_table.decor_mut().set_prefix(desc);
+        }


Is there a reason these can't call into apply_optional and friends?

Huh, right, I guess I didn't really consider that this is also a table we could just apply_optional_* into.

Would you like me to break this init commit out into a separate PR? Then we can land the rest with the init stuff missing.

cargo-dist/src/init.rs

This adds support for building Mac .pkg installers. This current implementation is intentionally a bit simplistic: it produces one .pkg per package in the user's workspace, without any special branding applied. We currently only allow the app identifier and install location to be configured, but we can consider adding further customization in the future.

mistydemeo requested a review from Gankra August 12, 2024 23:23

mistydemeo force-pushed the pkg branch from 1ed63cb to 12251b9 Compare August 13, 2024 00:23

mistydemeo force-pushed the pkg branch from 12251b9 to 4f811b6 Compare August 22, 2024 00:43

Gankra requested changes Aug 22, 2024

View reviewed changes

mistydemeo force-pushed the pkg branch 7 times, most recently from aa75c73 to ebabd6a Compare August 27, 2024 21:54

mistydemeo mentioned this pull request Aug 27, 2024

feat: support signing pkgs #1376

Draft

mistydemeo force-pushed the pkg branch from ebabd6a to faca69b Compare August 27, 2024 22:18

Gankra requested changes Aug 28, 2024

View reviewed changes

mistydemeo force-pushed the pkg branch 2 times, most recently from 5db9799 to 5b7bcd5 Compare August 28, 2024 18:56

mistydemeo force-pushed the pkg branch 2 times, most recently from dbd6f62 to 7b34136 Compare August 28, 2024 20:05

mistydemeo added 2 commits August 28, 2024 14:17

feat: add pkg to backend init

e5ba17f

tests: add pkg to gallery

43dd140

mistydemeo force-pushed the pkg branch from 7b34136 to 43dd140 Compare August 28, 2024 21:17

Gankra approved these changes Aug 28, 2024

View reviewed changes

Gankra merged commit 3f58b8e into main Sep 3, 2024
16 checks passed

Gankra deleted the pkg branch September 3, 2024 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Mac .pkg installer #1312

feat: Mac .pkg installer #1312

mistydemeo commented Aug 12, 2024 •

edited

Loading

Gankra Aug 22, 2024

mistydemeo Aug 23, 2024

Gankra Aug 22, 2024

mistydemeo Aug 22, 2024

Gankra Aug 22, 2024

mistydemeo Aug 22, 2024

Gankra Aug 22, 2024

mistydemeo Aug 22, 2024

Gankra Aug 22, 2024

Gankra Aug 22, 2024

mistydemeo Aug 23, 2024

mistydemeo Aug 27, 2024

Gankra left a comment

Gankra Aug 28, 2024

mistydemeo Aug 28, 2024

mistydemeo Aug 28, 2024

feat: Mac .pkg installer #1312

feat: Mac .pkg installer #1312

Conversation

mistydemeo commented Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistydemeo commented Aug 12, 2024 •

edited

Loading