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

Fix extensions nuget pipeline #629

Merged
merged 6 commits into from
Jan 4, 2024
Merged

Conversation

YUNQIUGUO
Copy link
Contributor

@YUNQIUGUO YUNQIUGUO requested a review from a team as a code owner January 4, 2024 00:12
wenbingl
wenbingl previously approved these changes Jan 4, 2024
@@ -122,6 +125,7 @@ jobs:

- script: |
ORT_EXTENSIONS_LOCAL_POD_PATH=$(Build.BinariesDirectory)/pod_staging \
EXCLUDE_MACOS_TARGET=true \
Copy link
Contributor

Choose a reason for hiding this comment

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

out of interest if we don't build the macos frameworks, why do we need to explicitly exclude them?

that seems like it will be a painful experience if someone is trying to do a local build with a subset of platforms

Copy link
Contributor Author

@YUNQIUGUO YUNQIUGUO Jan 4, 2024

Choose a reason for hiding this comment

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

in our current test app setup, the Podfile by default will include both macos/ios testing targets so we can share one single testing app.

And specifying this variable is just to let the CI test project pass if MacOS arch not included in the current build /using a subset of platforms

If a user doing a local build with their own project with correctly specified supported platform sets in the Podfile, wouldn't need to explicitly setting this variable.

Copy link
Contributor Author

@YUNQIUGUO YUNQIUGUO Jan 4, 2024

Choose a reason for hiding this comment

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

more natural way would be I can modify to include_macos_target if we build for macos:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment to the script for pod install with this info?

something along the lines of 'we exclude macos in the build_framework.py command in this CI but the test app's podfile is setup for all platforms, and due to that we have to explicitly exclude the macos target when installing the pod for the test app'. not sure that's completely accurate so please adjust as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I added a note. also added for another place in CI.yml file for the case where we only build for a subset of platforms.

@YUNQIUGUO YUNQIUGUO merged commit b072e94 into main Jan 4, 2024
35 checks passed
@YUNQIUGUO YUNQIUGUO deleted the yguo/fix-extensions-nuget-pipeline branch January 4, 2024 03:36
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.

3 participants