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

Print warning, if full version of app and model don't match #1280

Closed
hannobraun opened this issue Oct 27, 2022 · 2 comments · Fixed by #1300
Closed

Print warning, if full version of app and model don't match #1280

hannobraun opened this issue Oct 27, 2022 · 2 comments · Fixed by #1300
Labels
good first issue Good for newcomers type: feature New features and improvements to existing features

Comments

@hannobraun
Copy link
Owner

Currently, if the version of the Fornjot app doesn't match the version of Fornjot that a model uses, this will result in an error when the model is loaded. This works well enough for released versions, but the it might lead to confusion when in-development versions are involved. This might be the case, if a user tries to load an example model from this repository using a released version of the Fornjot app.

The version is determined at build time in the fj crate's build.rs. From there, it is included in the fj crate, and the statics defined there are then used at model-load-time in fj-host to check for a mismatch and potentially generate the error. Two different versions are determined and included in fj: The package version (something like 1.2.3), and a "full" version which includes more information, like the Git commit hash. Only the package version is used for the version check.

It would be too restrictive to use the full version as the basis for the version mismatch check, but since a mismatch there might cause mysterious problems, it's probably best to print a warning, explaining the situation. I'm not sure what the ideal solution is, but for a start, it would probably be enough to log a warning-level log message to stderr.

Labeling https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, as this only requires surface-level knowledge of Fornjot, and I've laid out all the moving parts here.

@hannobraun hannobraun added type: feature New features and improvements to existing features good first issue Good for newcomers topic: host labels Oct 27, 2022
@zthompson47
Copy link
Contributor

Hello! I put together a PR for this issue that prints a warning to stderr.

Testing is interesting because there is a new symbol in the shared library that renders all old models incompatible:

Error receiving updated shape: Failed to load the Fornjot version that the model uses
- Is your model using the `fj` library? All models must!
- Was your model created with a really old version of Fornjot?

Caused by:
    /home/zach/fornjot/models/shape/target/debug/libshape.so: undefined symbol: version_full

To test, I made a temporary clone of the repository with the updated symbol, and made a commit to get a different full version. I then used that repository in the Cargo.toml of the model crate to trigger a full version mismatch.

Is there any need for automated tests here? I suppose it might be tricky and/or unnecessary (at least for now).

@hannobraun
Copy link
Owner Author

Thank you for looking into this, @zthompson47!

Your approach to testing sounds very reasonable. I don't think we need an automated test for this. If someone came up with a reasonable one, I wouldn't say no, but I think this falls into this quadrant of testing where the test would be hard to write but the benefit would be low. Not worth losing sleep over, in my opinion.

I'll take a look at your pull request now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: feature New features and improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants