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

ENH: Add new dependencies to SlicerMorph.json #2101

Closed
wants to merge 2 commits into from

Conversation

smrolfe
Copy link
Contributor

@smrolfe smrolfe commented Nov 4, 2024

Adds Sandbox and SurfaceMarkup as dependencies to the SlicerMorph extension.

Adds Sandbox and SurfaceMarkup as dependencies to the SlicerMorph extension.
@@ -1,6 +1,10 @@
{
"$schema": "https://raw.githubusercontent.com/Slicer/Slicer/main/Schemas/slicer-extension-catalog-entry-schema-v1.0.0.json#",
"build_dependencies": ["SegmentEditorExtraEffects"],
"build_dependencies": [
Copy link
Contributor

Choose a reason for hiding this comment

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

https://slicer.readthedocs.io/en/latest/developer_guide/extensions.html#extension-catalog-entry-file

These don’t appear to be build dependencies but rather runtime dependencies? SurfaceMarkup is already specified to be installed when running the PlaceLandmarkGrid module:
https://github.com/SlicerMorph/SlicerMorph/blob/40407085db40774fd976a0bac4c6c287f49b5576/PlaceLandmarkGrid/PlaceLandmarkGrid.py#L48-L64

Copy link
Contributor

@jamesobutler jamesobutler Nov 4, 2024

Choose a reason for hiding this comment

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

Reviewing the Slicer ExtensionsIndex pull request template it appears this change would put it inconsistent with the extension’s listed dependencies in its top level CMakeLists.txt.

- [ ] Content of submitted json file is consistent with the top-level CMakeLists.txt file in the repository (dependencies, etc. are the same)

https://github.com/SlicerMorph/SlicerMorph/blob/40407085db40774fd976a0bac4c6c287f49b5576/CMakeLists.txt#L13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jamesobutler, these are runtime dependencies and I agree it would be better to install at runtime.

@smrolfe smrolfe closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants