ExtensionsIndex icon indicating copy to clipboard operation
ExtensionsIndex copied to clipboard

ENH: Add URL verification to s4ext file check

Open lassoan opened this issue 4 years ago • 6 comments

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

lassoan avatar Aug 04 '21 02:08 lassoan

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 ...

jcfr avatar Aug 04 '21 03:08 jcfr

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).

lassoan avatar Aug 04 '21 03:08 lassoan

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 '<extensionName>.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.

jcfr avatar Aug 04 '21 06:08 jcfr

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?

lassoan avatar Aug 04 '21 20:08 lassoan

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

    • This allow the "build tool" (e.g poetry, flit, setuptools, ...) to use these to create the wheel.
    • This is documented in https://www.python.org/dev/peps/pep-0621/#example
  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

jcfr avatar Aug 04 '21 21:08 jcfr

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.

lassoan avatar Aug 05 '21 02:08 lassoan