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

H5P CLI tool may cause confusion by ignoring library versions #55

Open
otacke opened this issue Aug 3, 2023 · 7 comments
Open

H5P CLI tool may cause confusion by ignoring library versions #55

otacke opened this issue Aug 3, 2023 · 7 comments
Assignees

Comments

@otacke
Copy link
Contributor

otacke commented Aug 3, 2023

When setting up libraries and fetching dependencies, the H5P CLI tool may cause confusion.

For instance, H5P.Column currently references H5P.Agamotto 1.5 as a soft dependency in semantics.json. The H5P CLI tool will fetch the master branch of H5P.Agamotto which already holds the 1.6 version, but it will neither notify the developer about that discrepancy (and suggest taking care of this by looking for the correct tag/commit) nor fail as a particular version of a library cannot be loaded like regular H5P integrations would. The H5P CLI tool will treat H5P.Agamotto 1.6 as if it was H5P.Agamotto 1.5.

That will work in many cases and one will not even notice anything, but it can cause confusion at least:

  • There may be breaking changes between subcontent versions that one is not aware of, e.g. a conflicting semantics structure or even logic changes.
  • The master branch commonly holds development versions which frequently contain bugs.
  • If the H5P CLI tool supported upgrading existing content to test the procedure, this would likely fail.
  • The content that's generated by the H5P CLI tool will be invalid and cannot be used on regular H5P integrations. Yes, that's not supposed to happen anyway, but people use this method to share their beta versions with testers, etc.
@devland
Copy link
Collaborator

devland commented Aug 3, 2023

Hey, Oliver.
Good catch.

As a temporary workaround until this is fixed you can setup specific versions of libraries by running something like
h5p setup <library> [version] where the "version" is the specific git repo tag you're looking to setup.
This will force the code to pull that specific version for the main library and also do the same for its dependencies.

@otacke
Copy link
Contributor Author

otacke commented Aug 3, 2023

I am not using the H5P CLI tool for development (yet) :-D

@devland devland self-assigned this Aug 4, 2023
@devland
Copy link
Collaborator

devland commented Aug 4, 2023

#58 fixes setup command when fetching specific versions.

That being said, because some h5p libraries are missing tags for some release versions, changing the default setup command to make it strict in terms of what versions are pulled would sometimes make it impossible to complete the setup.

For example, h5p-font-awesome is missing the “4.5.6” tag although currently sitting at version 4.5.6.
This makes the setup command for any library that requires h5p-font-awesome 4.5.6 crash with the fatal: Remote branch 4.5.6 not found in upstream origin error when a specific version is requested for the main library.

I've updated the commands.md file to describe the default behavior of the setup function and how it uses master branches when no specific version is requested.

@otacke
Copy link
Contributor Author

otacke commented Aug 4, 2023

@devland I am well aware that this is an organizational problem of missing tags (https://github.com/boyum/pack-h5p-action is suffering from the same issue, though there it seems even more troublesome as it is supposed to build packages for production). It might be addressed with a github action that automatically tag a version once the library version changes, but that's up to the maintainer, of course.

That's why I mentioned the option to at least warn the developer that the version referenced by the content type does not match the version on the master branch - or to fail as an H5P integration would while requiring attention by the developer to fix this himself/herself but not violating the H5P specification.

@falcon-git
Copy link
Member

I agree with @otacke. We should at least check and warn the user if we have set up an incorrect version. Should we also consider using the stable brach by default if it exists? This would make sense if we assume that we are mostly not fetching libraries to make changes to them but fetching them as dependencies for the code we are making changes to...

@otacke
Copy link
Contributor Author

otacke commented Nov 7, 2023

It feels similar to what happens if you install a library on Linux and some dependency cannot be resolved. You will be presented with options (e.g. keep the existing version, change the repository source, ignore, ...)

Similarly, if the master branch does not hold the version that is referenced, one could check what options exist depending on what the repository supplies and then display a dialog for the developer to choose from. Something like:

Cannot fetch H5P.Foo 1.1 which is referenced by H5P.Bar 1.0 from master branch. How would you like to proceed:
T) Use commit that was tagged 1.1
S) Use stable branch
R) Use release branch
I) Ignore and use master branch
A) Abort

T) Would require to check the repository for common tag name patterns (something along the lines of /v?(/d./d)(./d)?/) and determine the commit. May not be available.
S/R) Would require to check for the respective branch. May not be available.
I/A) Would always be available.

One could introduce a flag for the preferred resolution order, e.g. --resolve TA for try to use the tagged version if available first and abort otherwise. Don't know what a good default would be.

I know, this means some effort ...

@otacke
Copy link
Contributor Author

otacke commented Feb 22, 2024

Just ran into that trap myself.

H5P CLI fetched Interactive Video 1.26.32 from the master branch. The release version is 1.26.30. Shared some demo content that would not run on Lumi. Why? Lumi still uses a core version < 1.26 which is fine for IV 1.26.30, but not for IV 1.26.32. Had to waste some time to figure out what the problem actually was. :-(

Yes, Lumi needs to upgrade the H5P core version, problem solved. Still, as this issue's title states: "H5P CLI tool may cause confusion by ignoring library versions".

And just wondering: Shouldn't h5p/h5p-interactive-video@6f1fd34 (changing the required version of H5P core) have been accompanied with a bump of the minor version of Interactive Video to 1.27.0?

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

No branches or pull requests

3 participants