eclipse-equinox / p2

Eclipse Public License 2.0
14 stars 39 forks source link

Add support for bundle URL types in macOS launcher in product files #533

Closed sratz closed 1 week ago

sratz commented 1 month ago

Adds CFBundleURLTypes entries to the Info.plist dictionary.

Contributes to https://github.com/eclipse-platform/eclipse.platform.ui/issues/1901.

github-actions[bot] commented 1 month ago

Test Results

  375 files  ±0    375 suites  ±0   41m 30s :stopwatch: -31s 1 895 tests +2  1 892 :white_check_mark: +2  3 :zzz: ±0  0 :x: ±0  6 685 runs  +6  6 676 :white_check_mark: +6  9 :zzz: ±0  0 :x: ±0 

Results for commit 32e5debf. ± Comparison against base commit e1b554d8.

:recycle: This comment has been updated with latest results.

sratz commented 1 week ago

Thanks @sratz also for your patience on this one.

Thanks for the review!

I have pushed a few clean-ups/some streamlining to your branch. Here also the remark from PDE: I don't think it is necessary to be consistent with old styles, especially for internals (I'm actually in favor of the opposite: modernize the code at every occasion).

If you are fine with the additions, I'll squash and submit them tomorrow.

Thanks for taking the time to improve the code. Looks good!

This also looks fine to me, but I couldn't test it in action. I assume you are confident that this is the right general way. At least to me it looks sane.

We also ran an end-to-end test on this part of the functionality:

--> The resulting macOS package contains the correct Info.plist file with the correct syntax and the URI schemes in the package are correctly recognized by Apple's launch services.

So this can also be merged 👍


(*) Tycho needs a small adjustment as well: It wraps IProductDescriptor, so needs to adopt the new functionality:

diff --git a/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java b/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java
index 734d5b4dc..2d7a5137c 100644
--- a/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java
+++ b/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java
@@ -28,6 +28,7 @@ import org.eclipse.equinox.internal.p2.publisher.eclipse.ProductContentType;
 import org.eclipse.equinox.p2.metadata.IInstallableUnit;
 import org.eclipse.equinox.p2.metadata.IVersionedId;
 import org.eclipse.equinox.p2.repository.IRepositoryReference;
+import org.eclipse.equinox.p2.publisher.eclipse.IMacOsBundleUrlType;
 import org.eclipse.tycho.ArtifactType;
 import org.eclipse.tycho.Interpolator;
 import org.eclipse.tycho.core.VersioningHelper;
@@ -291,4 +292,9 @@ class ExpandedProduct implements IProductDescriptor {
         return defaults.getVM(os);
     }

+    @Override
+    public List<IMacOsBundleUrlType> getMacOsBundleUrlTypes() {
+               return defaults.getMacOsBundleUrlTypes();
+       }
+
 }

But we will take care of the Tycho part as soon as Tycho consumes Eclipse 2024-09 P2.

HannesWell commented 1 week ago

This also looks fine to me, but I couldn't test it in action. I assume you are confident that this is the right general way. At least to me it looks sane.

We also ran an end-to-end test on this part of the functionality:

* Installed a snapshot of `org.eclipse.equinox.p2.publisher.eclipse:1.6.200-SNAPSHOT` to local maven repo

* Consumed this version in a Tycho 5.0.0-SNAPSHOT build (*)

* Created a `.product` file with the new PDE code

* Ran `publish-products` using Tycho

--> The resulting macOS package contains the correct Info.plist file with the correct syntax and the URI schemes in the package are correctly recognized by Apple's launch services.

So this can also be merged 👍

Great! Then, lets have this one in. I just squashed the commits and will submit it once the build passed again.