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

build: remove dead symlink from MAS build #24158

Merged
merged 5 commits into from Jun 16, 2020

Conversation

MarshallOfSound
Copy link
Member

Fixes #23828

This fixes the stray symbolic link and adds a script to ensure it doesn't happen again

Notes: Fixed mac app store rejection notice for invalid symbolic link in bundle.

@nornagon
Copy link
Member

This is a reland of #23831?

@MarshallOfSound
Copy link
Member Author

@nornagon Yes but with a more correct fix for the gn check failures, the MAS build now truly doesn't depend on any parts of the crash component

@MarshallOfSound MarshallOfSound merged commit 09c0ee8 into master Jun 16, 2020
@release-clerk
Copy link

release-clerk bot commented Jun 16, 2020

Release Notes Persisted

Fixed mac app store rejection notice for invalid symbolic link in bundle.

@MarshallOfSound MarshallOfSound deleted the fix-dead-symlink-on-mas-2 branch June 16, 2020 21:20
@trop
Copy link
Contributor

trop bot commented Jun 16, 2020

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jun 16, 2020

I have automatically backported this PR to "10-x-y", please check out #24166

@deepak1556
Copy link
Member

deepak1556 commented Jun 17, 2020

Some crash symbols still leak through by

//electron:electron_lib --[private]-->
//electron/chromium_src:chrome --[public]-->
//chrome/common:common --[private]-->
//components/crash/core/app:app

if symbols should be completely removed in MAS build then this path should be addressed.

@nornagon
Copy link
Member

nornagon commented Jun 17, 2020

I don't think symbols are the issue here, I think it's API calls? So as long as we're not calling things that don't work in MAS, we're OK.

@deepak1556
Copy link
Member

I am not sure too, @MarshallOfSound can you please clarify the end goal of conditionally including those deps. If symbols are not an issue, I would like to add //components/crash/core/app back and remove the nogncheck

@MarshallOfSound
Copy link
Member Author

@deepak1556 It's a combination. The original goal was to completely remove the crash component as a dependency to guarantee that MAS build can't call anything in there that straight up won't work. If there's a dependency chain I missed I can follow up to remove that.

The actual problem this PR solves is twofold.

(1) The Helpers folder needs to not exist on MAS builds, this requires changing some GN config
(2) The MAS build needs to not end up binding to, or calling, any crash methods (so this introduces some new noop / stub methods)

I think the dependency chain you posted can be removed by patching out the dependency for mas builds

@deepak1556
Copy link
Member

Thanks for the context!

Removing the dependency chain from //chrome is a patch that we have to maintain always. With the electron crash api sources, Helpers framework and crashpad_handler_syms excluded, can we just build the deps unconditionally ?

@MarshallOfSound
Copy link
Member Author

With the electron crash api sources, Helpers framework and crashpad_handler_syms excluded, can we just build the deps unconditionally ?

The Helpers and syms are completely excluded but the electron_api_crash_reporter files and a few others are left in and we rely on MAS_BUILD defined checks to ensure we aren't doing The Wrong Thing in a mas build. It would be way safer to ensure it's impossible link those symbols into the MAS build accidentally. I don't know how big of a patch that would end up looking like though.

@deepak1556
Copy link
Member

Hmm, this is a unique case where it is not completely necessary to remove the symbols but at the same time want to ensure headers are behind #if defined. I am fine with ignoring the symbols coming from //chrome, I don't think a patch is worth it.

@davidwinter
Copy link

Will this be backported to v9?

MarshallOfSound added a commit that referenced this pull request Jun 22, 2020
* build: remove dead symlink from MAS build

* chore: new out cache

* build: fixup gn check

* Update node_main.cc

* chore: fix lint
@trop
Copy link
Contributor

trop bot commented Jun 22, 2020

@MarshallOfSound has manually backported this PR to "9-x-y", please check out #24238

MarshallOfSound added a commit that referenced this pull request Jun 22, 2020
* build: remove dead symlink from MAS build

* chore: new out cache

* build: fixup gn check

* Update node_main.cc

* chore: fix lint
sentialx pushed a commit to sentialx/electron that referenced this pull request Jul 30, 2020
* build: remove dead symlink from MAS build

* chore: new out cache

* build: fixup gn check

* Update node_main.cc

* chore: fix lint
sentialx pushed a commit to sentialx/electron that referenced this pull request Apr 8, 2021
* build: remove dead symlink from MAS build

* chore: new out cache

* build: fixup gn check

* Update node_main.cc

* chore: fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electron 9.0.0: Mac App Store rejection - ITMS-90332: Invalid Symbolic Link
4 participants