This repository has been archived by the owner on Jan 18, 2024. It is now read-only.
[xdl] Remove call to check-dynamic-macros #1933
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wkozyra95
approved these changes
Apr 20, 2020
dsokal
approved these changes
Apr 20, 2020
|
It wouldn't be a requirement for a shell app to have this file. We could remove the check from |
|
I think we should do sth with this file now - either remove it or run the check somewhere else. Otherwise, I assure you that we'll forget about it and it'll cause someone's headache in the future. |
|
I will be happy to remove it! |
|
This also means that my PR may be reverted - https://github.com/expo/expo/pull/7607/files :D |
This was referenced Apr 20, 2020
sjchmiela
added a commit
to expo/expo
that referenced
this pull request
Apr 20, 2020
# Why Follow-up to expo/expo-cli#1933. # How Since we won't use `check-dynamic-macros-android.sh` when building standalone apps and there is no other place we use it we can remove it. Let's believe in `expotools`! # Test Plan I will cherry-pick that commit to `sdk-37` and push a new image to Android staging.
sjchmiela
added a commit
to expo/expo
that referenced
this pull request
Apr 20, 2020
# Why Follow-up to expo/expo-cli#1933. # How Since we won't use `check-dynamic-macros-android.sh` when building standalone apps and there is no other place we use it we can remove it. Let's believe in `expotools`! # Test Plan I will cherry-pick that commit to `sdk-37` and push a new image to Android staging.
sjchmiela
added a commit
to expo/turtle
that referenced
this pull request
Apr 20, 2020
### Motivation and Context See expo/expo-cli#1933 and expo/expo#7919. ### Description Updates XDL so it includes change introduced in expo/expo-cli#1933.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
This check has been added in #98 as an effort to ensure that the shell apps contain the pregenerated macros as expected.
I'm not sure this is needed anymore — from my point of view the shell app creation process is very clear now (one, simple script),
expotoolsconsistently generate the macros and I don't think XDL should be tied that closely to internals of the shell app.If we wanted to, we could replace this check, run for every build, with a post-shell-app-build check in
expo/exporepo, where the shell apps are created.How
Removed spawning of the aforementioned script.
Test plan
As this only removed the call and doesn't add anything I don't see it breaking anything.
It's unlikely we will need to rebuild shell apps for old SDKs, where
expotoolsmay have not existed yet and even if so, since all current shell apps do contain the macros, I think it should be safe to assume that any additions tosdk-XXbranches won't break the macros generation process.