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

Switch extension index entry format from "s4ext" to "json" | ⚠️ Changes removed from main #7629

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Mar 13, 2024

⚠️ Changes originally introduced through this pull-request have been removed from main by force pushing. Corresponding changes are superseded by #7709 ⚠️

Rational:

  • Commit message was incorrect
  • Changes were integrated using Squash & Merge instead of Rebase & Merge

In response to discussions during the Slicer developer hangout on March 12th, we have decided to revamp the organization of extension metadata in the extensions index, transitioning from the s4ext format to json.


Implements changes to the Extensions build-system, transitioning from .s4ext files to .json files for extension catalog entries. Additionally, it updates the ExtensionWizard and module templates to reflect this change and supports contributing .json files to the ExtensionsIndex.

As the category is now expected to be defined in the extension catalog entry file, the support for setting the "Category" directly from the ExtensionWizard UI has been removed.

While a .s4ext file is still generated in the built-tree and included in the extension package, this is considered an implementation detail to be addressed in subsequent commits.

The motivations behind these changes include:

  • Eliminate redundant and unused information from the "description" file.
  • Simplify programmatic parsing of the "description" files
  • Decouple the metadata organized in the extension CMakeLists.txt from
    the ones organized in this repository and used to drive the build of extensions.
  • Enable Slicer maintainers to define and update the extension "category"
    and "enabled" metadata independently of the upstream extension sources.

It removes the parsing and handling of metadata already defined in the extension CMakeLists.txt by the extension index build-system.

The metadata values now extracted from the <extensionname>.s4ext file locally generated in the extension build tree are the following:

  • "contributors"
  • "depends" (runtime dependencies)
  • "description"
  • "iconurl"
  • "homepage"
  • "screenshots"

The only metadata values propagated from the Slicer/ExtensionsIndex files now formatted as JSON files (<extensionname.json) are the following:

  • "scm", "scmurl" and "scmrevision"
  • "category"
  • "build_dependencies"
  • "build_subdirectory"
  • "enabled"

The extension name is still derived from the catalog entry filename.

Related:


Extension description may now use a limited subset1 of HTML tags and attributes.

Related pull requests:

Footnotes

  1. https://github.com/matthiask/html-sanitizer#settings

@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch 2 times, most recently from b9f087c to 9722894 Compare March 13, 2024 22:06
Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jcfr the changes look good to me. I've just added a few small comments.

@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch 3 times, most recently from 751498c to eeba6b0 Compare March 14, 2024 03:15
@lassoan

This comment was marked as outdated.

@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from eeba6b0 to 89a306e Compare March 19, 2024 16:57
@lassoan

This comment was marked as outdated.

@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from 89a306e to 7b07924 Compare March 19, 2024 21:57
@jcfr

This comment was marked as outdated.

Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you, I've added some comments. I'm OK with most of them left unchanged, the only thing that I feel strongly about is that we should include the schema name in the file. I would rather not add lots of json files that are not clear what contain, especially because neither the filename, nor the file extension or file content is specific enough to confidently tell that it is an extensions catalog entry.

Docs/developer_guide/extensions.md Show resolved Hide resolved
Extensions/CLIExtensionTemplate.json Show resolved Hide resolved
Extensions/CLIExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/CLIExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/LoadableExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/ScriptedLoadableExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/SuperBuildExtensionTemplate.json Outdated Show resolved Hide resolved
@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch 2 times, most recently from 0404746 to 3a58c0b Compare March 20, 2024 20:05
lassoan
lassoan previously approved these changes Mar 20, 2024
Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you, it looks good, just marked a few trivial things. If there is any trouble with removing "NA" then it is fine to leave it there.

Docs/developer_guide/extensions.md Outdated Show resolved Hide resolved
Docs/developer_guide/extensions.md Outdated Show resolved Hide resolved
Docs/developer_guide/extensions.md Outdated Show resolved Hide resolved
Extensions/CLIExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/LoadableExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/ScriptedLoadableExtensionTemplate.json Outdated Show resolved Hide resolved
Extensions/SuperBuildExtensionTemplate.json Outdated Show resolved Hide resolved
@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from 3a58c0b to 5c79b1f Compare March 21, 2024 01:22
@jcfr
Copy link
Member Author

jcfr commented Mar 21, 2024

If I understand correctly, we should remove the category from here then.

Ditto.

This should be addressed in the commit ENH: Ensure extension category from catalog entry file is used, I still need to test the approach.

Also need to display warnings (when using Slicer >= 5.8) to suggest removal of EXTENSION_CATEGORY and EXTENSION_ENABLED in extension CMakeLists.txt if the corresponding variables are set.

@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch 2 times, most recently from 14c8607 to f96c71c Compare March 28, 2024 21:02
@lassoan
Copy link
Contributor

lassoan commented Mar 28, 2024

@jcfr let me know if this is ready for final review

@jcfr
Copy link
Member Author

jcfr commented Mar 29, 2024

This is ready for review.

That said, I still need to update the ExtensionIndex based in the new schema and create PR on existing Extension.

@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from f96c71c to 0a243fe Compare April 11, 2024 16:02
@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from 0a243fe to 3f37a26 Compare April 20, 2024 05:31
@jcfr jcfr requested review from lassoan and pieper April 23, 2024 15:24
@jcfr
Copy link
Member Author

jcfr commented Apr 23, 2024

Let me know if this is ready for final review

@lassoan This is now ready for final review.

@jcfr jcfr requested a review from jamesobutler April 23, 2024 19:19
Anticipating the simplification of the metadata used to describe an extension
Simplifies handling of metadata
catalog entry, this removes the parsing and handling of metadata already defined
in the extension CMakeLists.txt by the extension index build-system.

The following metadata values are now extracted from the `<extensionname>.s4ext`
file locally generated in the extension build tree:
- "category"
- "contributors"
- "enabled"
- "depends" (runtime dependencies)
- "description"
- "iconurl"
- "homepage"
- "screenshots"

The only metadata values propagated from the Slicer/ExtensionsIndex files
are the following:
- "scm", "scmurl" and "scmrevision"
- "depends" (built-time dependencies)
- "build_subdirectory"

The extension name is still derived from the catalog entry filename.
@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from 3f37a26 to 5a7fc3d Compare April 23, 2024 19:52
jcfr and others added 3 commits April 23, 2024 16:47
This commit implements changes to the Extensions build-system,
transitioning from `.s4ext` files to `.json` files for extension
index entries. Additionally, it updates the ExtensionWizard and
module templates to reflect this change and supports contributing
`.json` files to the ExtensionsIndex.

As the category is now expected to be defined in the extension catalog
entry file, the support for setting the "Category" directly from
the ExtensionWizard UI has been removed.

While a .s4ext file is still generated in the built-tree and included
in the extension package, this is considered an implementation detail
to be addressed in subsequent commits.

The motivations behind these changes include:
* Eliminate redundant and unused information from the "description" file.
* Simplify programmatic parsing of the "description" files
* Decouple the metadata organized in the extension `CMakeLists.txt` from
  the ones organized in this repository and used to drive the build of extensions.
* Enable Slicer maintainers to define and update the extension "category"
  and "enabled" metadata independently of the upstream extension sources.
@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from 5a7fc3d to e6151f3 Compare April 23, 2024 20:48
pieper
pieper previously approved these changes Apr 23, 2024
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Impressive work Jc 👍

I don't see anything obviously wrong so if you are comfortable I think we should go ahead. We'll know for sure once we try it for real.

@jcfr
Copy link
Member Author

jcfr commented Apr 23, 2024

Thanks for the cursory check.

if you are comfortable I think we should go ahead

On the metroplex factory, I was able to locally build the index and publish an extension so we should be in good shape.

@jcfr jcfr enabled auto-merge (rebase) April 23, 2024 21:08
@jcfr jcfr disabled auto-merge April 23, 2024 21:14
@jcfr jcfr force-pushed the transition-extensions-index-build-system-from-s4ext-to-json branch from e6151f3 to 783f401 Compare April 23, 2024 21:20
@jcfr
Copy link
Member Author

jcfr commented Apr 23, 2024

While ensuring Slicer/ExtensionsIndex#2011 was also passing, I realized I missed one update1 to the schema, this is now fixed.

@pieper I would appreciate a re-approval

Footnotes

  1. https://github.com/Slicer/Slicer/compare/e6151f39214aa197d77b7a6b083d9c4c181ceb8e..783f4015450605930ae9d7f68cf3f1ea7d6418de

@jcfr jcfr requested a review from pieper April 23, 2024 21:22
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

👍

@pieper pieper enabled auto-merge (squash) April 23, 2024 22:25
@pieper pieper merged commit 59f9126 into Slicer:main Apr 23, 2024
5 checks passed
@jcfr jcfr changed the title Switch extension index entry format from "s4ext" to "json" Switch extension index entry format from "s4ext" to "json" | ⚠️ Changes removed from main Apr 24, 2024
@jcfr jcfr deleted the transition-extensions-index-build-system-from-s4ext-to-json branch April 24, 2024 02:42
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.

3 participants