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 URL verification to s4ext file check #1788

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lassoan
Copy link
Contributor

@lassoan lassoan commented Aug 4, 2021

The URL is requested from the server and verified that it returns with OK status (200).

Also added the option to specify filenames with wildcards. For example, this allows easily running URL checks on all files locally:

python.exe check_description_files.py --check-urls *.s4ext

The URL is requested from the server and verified that it returns with OK status (200).

Also added the option to specify filenames with wildcards. For example, this allows easily running URL checks on all files locally:

    python.exe check_description_files.py --check-urls *.s4ext
@jcfr
Copy link
Member

jcfr commented Aug 4, 2021

Thanks for working on this.

I spent some time working on this today as well and will add a follow up commit.

The additional checks I considered are:

  • extract user and repo from scmurl
  • download and parse cmakelists.txt to check that URL in the cmakelists are valid ...

@lassoan
Copy link
Contributor Author

lassoan commented Aug 4, 2021

It would be great if you could fix this circleci configuration - I don't know how to pip install "requests" package.

download and parse cmakelists.txt to check that URL in the cmakelists are valid

I would remove the s4ext file generation from CMake and instead ask the user to create a .s4ext (or .json or .yaml) manually and CMake would get all the info it needs. It could be easily made backward-compatible (if the CMake script does not find a suitable extension description file then it falls back to use CMake variables).

@jcfr
Copy link
Member

jcfr commented Aug 4, 2021

Re: fix circle ci
I may switch to GitHub Action

Re: remove generation of s4ext

To help revisit the current approach, here are few comments:

(1) we need to submit the following Metadata when uploading an extension: category, homepage, iconurl, description, scmurl, revision, screenshoturls, contributors, depends

(2) we need the following Metadata when building : scmurl, scmrevision, depends, build_subdirectory

Currently, the s4ext file fulfills two roles:

  • describing the extension for presentation on the extensions manager website
  • providing info for checking out and building the extension

There are few approaches to move forward:

Approach 1: decouple the two roles

This could be done by:

  1. including file ls named like <extensionName>.build.json in the index

    • this file would only have: scmurl, scmrevision, build_subdirectory, depends
  2. Having a file called slicer_extension.metadata.json at the route of every extension source tree

    • this would be the authoritative source for extension metadata

    • cmake would expect that file to exist and would extract info to create the package and upload info to the Slicer-packages server

To avoid keeping the 'depends' Metadata in sync between the build and metadata file ...

... we could always expect the build system to download the metadata file from the repo. That way, no duplication.

Approach 2: having a single file

The file slicer_extension.metadata.json would be copied to the index, renamed as '.metadata.jsonand updated to includescmurl, scmrevision, dependsand build_subdirectory`

Notes

  • we should be able to remove the need for `build_subdirectory' and instead have convenient package and package packageupload targets in the top level for superbuild extension.

@lassoan
Copy link
Contributor Author

lassoan commented Aug 4, 2021

I may switch to GitHub Action

I fully support this. Github actions might not be as sophisticated as CircleCI, but I find them much easier to use.

Re: remove generation of s4ext

It would be better if we didn't invent something from scratch. Can we do something similar that is done in the Python Package Index?

@jcfr
Copy link
Member

jcfr commented Aug 4, 2021

Can we do something similar that is done in the Python Package Index?

For python wheels, there are few aspects:

  1. describing the metadata in the source tree in the pyproject.toml file

  2. storing the metadata in the wheel package

If we would like to also adopt .toml format, we would have to include a toml parser since this is not supported in CMake.

Since starting with CMake 3.19, we have JSON support in CMake, adopting json would be a natural choice.

Describing metadata using json works well in the javascript ecosystem, looking at https://docs.npmjs.com/cli/v7/configuring-npm/package-json is also sensible.

Requirements:

  • extensible and versioned format
  • human and computer readable

@lassoan
Copy link
Contributor Author

lassoan commented Aug 5, 2021

Since starting with CMake 3.19, we have JSON support in CMake, adopting json would be a natural choice.

I agree, we should just use json. Both toml and metadata format has rules for json conversion (see https://www.python.org/dev/peps/pep-0566/#json-compatible-metadata and https://github.com/toml-lang/toml/wiki#converters), so we should be able to keep the same or very similar structure, field names, values, etc.

The Python metadata fields are quite limited compared to npm metadata file fields: single author, no explicit bugtracker URL, quite limited repository specification, etc. That said neither of them contain everything that we need (e.g. acknowledgments, funding sources, icon and screenshot URLs, ...), so we cannot just adopt one or the other, but we can just get inspirations and adopt some conventions.

There seems to be a similar situation in duplication of metadata in Python wheels and source: https://packaging.python.org/specifications/core-metadata/#dynamic-multiple-use. However, I could not exactly understand their problem or their solution.

@jcfr jcfr added the Type: Enhancement Improvement to functionality label Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improvement to functionality
Development

Successfully merging this pull request may close these issues.

2 participants