Skip to content

Revert "Disable Integration job on main branch" #957

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

Merged
merged 3 commits into from
Jan 25, 2022

Conversation

Saadnajmi
Copy link
Collaborator

Reverts #947
Closes #948

@Saadnajmi Saadnajmi requested a review from alloy as a code owner January 15, 2022 02:55
@pull-bot
Copy link

pull-bot commented Jan 15, 2022

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against e4047b4

@Saadnajmi
Copy link
Collaborator Author

I'm actually beginning to think the fix might be that react-native-test-app needs to update their own podfile.lock / update their template with react-native-macos 0.66. @tido64 thoughts?

@tido64
Copy link
Member

tido64 commented Jan 17, 2022

I'm actually beginning to think the fix might be that react-native-test-app needs to update their own podfile.lock / update their template with react-native-macos 0.66. @tido64 thoughts?

Can you add rm macos/Podfile.lock before this line:

pod install --project-directory=macos

The repo is currently at 0.64. Updating it for 0.66 would break everything else. We need to remove this file regardless because it's not forwards-compatible. I updated the integration script in RNTA, but forgot to do it here as well.

@Saadnajmi
Copy link
Collaborator Author

Saadnajmi commented Jan 17, 2022

I'm actually beginning to think the fix might be that react-native-test-app needs to update their own podfile.lock / update their template with react-native-macos 0.66. @tido64 thoughts?

Can you add rm macos/Podfile.lock before this line:

pod install --project-directory=macos

The repo is currently at 0.64. Updating it for 0.66 would break everything else. We need to remove this file regardless because it's not forwards-compatible. I updated the integration script in RNTA, but forgot to do it here as well.

Thanks!

@Saadnajmi Saadnajmi closed this Jan 17, 2022
@Saadnajmi Saadnajmi reopened this Jan 17, 2022
@Saadnajmi
Copy link
Collaborator Author

The latest CI run fails from the familiar clockid_ redefinition error, which we fixed in RN-macOS with a postinstall script in our template po file while we wait for @amgleitman 's fix in folly to merge.

We can probably add that script into RNTA (though I recall RNTA removing all their postinstall scripts recently so they may not want that), but it does make me think of the question of dependencies again. With every RN-macOS release, will this check only pass once we've published a stable version package and RNTA has had a chance to update themselves? If so, should we keep it disabled on main where we may not be in a supported state?

@tido64
Copy link
Member

tido64 commented Jan 18, 2022

The latest CI run fails from the familiar clockid_ redefinition error, which we fixed in RN-macOS with a postinstall script in our template po file while we wait for @amgleitman 's fix in folly to merge.

Sorry for the inconvenience. Ideally, fixes like this should be hidden behind the one post-install script, react_native_post_install. I dunno why we decided to add another one that users have to go and add themselves. I've been advocating for more seamless upgrades, but things like this keep going through. Let me add the script real quick.

Edit: microsoft/react-native-test-app#704

We can probably add that script into RNTA (though I recall RNTA removing all their postinstall scripts recently so they may not want that), but it does make me think of the question of dependencies again. With every RN-macOS release, will this check only pass once we've published a stable version package and RNTA has had a chance to update themselves? If so, should we keep it disabled on main where we may not be in a supported state?

I wouldn't be so quick to throw this out. This isn't a required run so it failing doesn't block anyone. If the red-X is annoying, we can probably reduce it to a warning. However, it has caught many real issues and I think it would be better if we could be pro-active rather than reactive about fixing them.

@Saadnajmi
Copy link
Collaborator Author

The latest CI run fails from the familiar clockid_ redefinition error, which we fixed in RN-macOS with a postinstall script in our template po file while we wait for @amgleitman 's fix in folly to merge.

Sorry for the inconvenience. Ideally, fixes like this should be hidden behind the one post-install script, react_native_post_install. I dunno why we decided to add another one that users have to go and add themselves. I've been advocating for more seamless upgrades, but things like this keep going through. Let me add the script real quick.

Edit: microsoft/react-native-test-app#704

We can probably add that script into RNTA (though I recall RNTA removing all their postinstall scripts recently so they may not want that), but it does make me think of the question of dependencies again. With every RN-macOS release, will this check only pass once we've published a stable version package and RNTA has had a chance to update themselves? If so, should we keep it disabled on main where we may not be in a supported state?

I wouldn't be so quick to throw this out. This isn't a required run so it failing doesn't block anyone. If the red-X is annoying, we can probably reduce it to a warning. However, it has caught many real issues and I think it would be better if we could be pro-active rather than reactive about fixing them.

I didn't notice it was not required, thank you :D. I agree not to throw it out, my apologies :). I wanted to understand the dependency graph better, and having it fail so we can be proactive makes total sense!

@Saadnajmi
Copy link
Collaborator Author

@tido64 Any idea why the "validate manifest" script phase fails?

@Saadnajmi Saadnajmi mentioned this pull request Jan 19, 2022
4 tasks
@tido64
Copy link
Member

tido64 commented Jan 20, 2022

@tido64 Any idea why the "validate manifest" script phase fails?

It's a new thing I added to make sure that app.json is valid. It looks like it fails to run on Node 12. Sorry about that.

Edit: This should be fixed now. We should also bump Node to a later version. Latest LTS is now 16.

@tido64
Copy link
Member

tido64 commented Jan 20, 2022

Filed #975

@Saadnajmi Saadnajmi merged commit 5ad4444 into microsoft:main Jan 25, 2022
@Saadnajmi Saadnajmi deleted the enable-integration branch January 25, 2022 17:08
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.

[CI] Integration test fails on main
4 participants