Skip to content

Conversation

@fregante
Copy link
Member

@fregante fregante commented May 4, 2023

Xcode has static project file lists so if our files are dynamically created one needs to open Xcode and update the project.

This seems to add a link to the parent distribution folder so that only one item needs to be static.

Inspired by:

@fregante fregante added meta Related to Refined GitHub itself safari Related to Safari only labels May 4, 2023
@fregante fregante requested a review from sindresorhus May 4, 2023 10:07
@sindresorhus
Copy link
Member

I think we already tried symlink before. The problem is that it copies the whole distribution folder into Resources instead of the contents of the folder. So you end up with:

Refined GitHub Extension.appex/Contents/Resources/distribution/manifest.json

I think manifest.json has to be directly inside Resources.

@fregante
Copy link
Member Author

fregante commented May 4, 2023

Can you try building and running it in Safari macOS? It seems to work correctly in the iOS Simulator (I only manage to run the dev version on macOS if I uninstall RGH completely)

@fregante
Copy link
Member Author

fregante commented May 6, 2023

Well, this works on iOS somehow but it crashes Safari macOS.

I'll move the assets to Resources/assets instead so that just asset can be a link. However I'd still ask you to try to make Resources a symlink to distribution instead

@fregante fregante marked this pull request as draft May 6, 2023 16:45
@sindresorhus
Copy link
Member

I don't think Resources can be a symlink. Then we would have done that last time. I think the only other option is to go back to the script again (which has downsides I forgot): 9036cce#diff-f6596608b09878fdc971f0f15ca2f2b4fc694cf71f1908ddc5564994f9528706R242

@fregante
Copy link
Member Author

fregante commented May 7, 2023

The downsides of the scripts are that it doesn’t watch the FS so developing the extension in Safari means having to build it manually every time. Extension testing seems already broken compared to the default setup since Safari can’t see the extension until I uninstall the App Store version, usually the dev version automatically replaces it immediately.

@fregante fregante changed the title Meta: Use completely-dynamic file list in Safari Meta: Use dynamic file list in Xcode May 7, 2023
@fregante fregante marked this pull request as ready for review May 7, 2023 13:21
Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Tested on macOS Safari:

However today iOS decided to fail building here and on main 🤷‍♂️

Screenshot 4


/* Begin PBXBuildFile section */
1F9945322A07DB5E005BC236 /* resources in Resources */ = {isa = PBXBuildFile; fileRef = 1F9945312A07DB5E005BC236 /* resources */; };
1F9945332A07DB5E005BC236 /* resources in Resources */ = {isa = PBXBuildFile; fileRef = 1F9945312A07DB5E005BC236 /* resources */; };
Copy link
Member Author

Choose a reason for hiding this comment

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

Since distribution can't be changed, I added distribution/resources as a symlink:

Screenshot 3

@sindresorhus
Copy link
Member

However today iOS decided to fail building here and on main 🤷‍♂️

I'm getting this too. If you look at the full log, this is the problem:

/Users/sindresorhus/Library/Developer/Xcode/DerivedData/Refined_GitHub-dshkgxrkdsfhtxedicouzyfghibo/Build/Products/Debug-iphonesimulator/Refined GitHub Extension.appex: code object is not signed at all
In subcomponent: /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Refined_GitHub-dshkgxrkdsfhtxedicouzyfghibo/Build/Products/Debug-iphonesimulator/Refined GitHub Extension.appex/icon.png

@sindresorhus
Copy link
Member

Fixed in main.

@fregante
Copy link
Member Author

codesign worked for me eventually, I had to reset my keys in the keychain etc etc, hundreds of people complaining about that issue for years on Stackoverflow.

One also mentioned the possible incompatibility with folders called resources, I wonder if this PR triggered that.

@sindresorhus
Copy link
Member

Hmm, actually no. Now it complains about the manifest file (main builds fine though):

/Users/sindresorhus/Library/Developer/Xcode/DerivedData/Refined_GitHub-dshkgxrkdsfhtxedicouzyfghibo/Build/Products/Debug-iphonesimulator/Refined GitHub Extension.appex: code object is not signed at all
In subcomponent: /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Refined_GitHub-dshkgxrkdsfhtxedicouzyfghibo/Build/Products/Debug-iphonesimulator/Refined GitHub Extension.appex/manifest.json
Command CodeSign failed with a nonzero exit code

@sindresorhus
Copy link
Member

The extension contains:

resources
Info.plist
manifest.json
Refined GitHub Extension

However, a proper extension has all the resources in a Resources directory. manifest.json cannot be at the top-level. It should also be Resources, not resources (unclear if that matters though).

E3919AF727E61192009C4956 /* refined-github.js.LICENSE.txt in Resources */ = {isa = PBXBuildFile; fileRef = E3919AE627E61192009C4956 /* refined-github.js.LICENSE.txt */; };
E3919AF827E61192009C4956 /* refined-github.js.LICENSE.txt in Resources */ = {isa = PBXBuildFile; fileRef = E3919AE627E61192009C4956 /* refined-github.js.LICENSE.txt */; };
E3919AF927E61192009C4956 /* manifest.json in Resources */ = {isa = PBXBuildFile; fileRef = E3919AE727E61192009C4956 /* manifest.json */; };
E3919AFA27E61192009C4956 /* manifest.json in Resources */ = {isa = PBXBuildFile; fileRef = E3919AE727E61192009C4956 /* manifest.json */; };
Copy link
Member Author

@fregante fregante May 14, 2023

Choose a reason for hiding this comment

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

Double-check, this PR only moves the assets one level deep. Manifest.json is unchanged in Resources/manifest.json

I stupidly named this subfolder resources (for extra confusion 😅) though so you should be seeing Resources/resources/background.js

Comment on lines 240 to 244
E3919AC127E6107E009C4956 /* Resources */ = {
isa = PBXGroup;
children = (
E3919AEA27E61192009C4956 /* background.js */,
E3919AED27E61192009C4956 /* icon.png */,
1F9945312A07DB5E005BC236 /* resources */,
E3919AE727E61192009C4956 /* manifest.json */,
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Resources/resources/*
  • Resources/manifest.json

children = (
E3919AEA27E61192009C4956 /* background.js */,
E3919AED27E61192009C4956 /* icon.png */,
1F9945312A07DB5E005BC236 /* assets */,
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I checked out this PR
  2. Got codesign error again
  3. Renamed this subfolder to assets
  4. It builds now, both iOS and macOS
Screenshot 7

@sindresorhus
Copy link
Member

It builds now and the extension seems to work, but the options page is blank (tested on device and simulator):

Simulator Screenshot - iPhone 14 Pro Max - 2023-05-14 at 16 40 24

@fregante
Copy link
Member Author

fregante commented May 14, 2023

Thanks for checking! Fixed.

I'll make the options open in a new tab in a future release instead of a popup and also get the value from the manifest dynamically:

const optionsPage = browser.runtime.getManifest().options_ui?.page;

@sindresorhus sindresorhus merged commit 2f8e86a into main May 14, 2023
@sindresorhus sindresorhus deleted the dynamic-safari branch May 14, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

meta Related to Refined GitHub itself safari Related to Safari only

Development

Successfully merging this pull request may close these issues.

2 participants