ome / ome-model

OME model (specification, code generator, implementation)
Other
13 stars 26 forks source link

maven: Correct filtering of model version #84

Closed rleigh-codelibre closed 6 years ago

rleigh-codelibre commented 6 years ago

I'm not sure when this regression occurred. It might date back to the repository decoupling. Whenever it happened, this is the simple fix to enable resource filtering and package up the filtered resource with the correct model version. Resource moved to src/main/resources where maven expects it by default.

Addendum: and a workaround for Sphinx added to make Travis work. You've probably already seen this in other projects.

Testing:

Without this PR:

% mvn
% mkdir check
% cd check
% jar xf ../ome-xml/target/ome-xml-5.6.4-SNAPSHOT.jar
% cat java/ome/xml/model/omemodel.properties
schema.version = ${ome.model.schemaver}

With this PR merged:

% mvn
% mkdir check
% cd check
% jar xf ../ome-xml/target/ome-xml-5.6.4-SNAPSHOT.jar
% cat ome/xml/model/omemodel.properties
schema.version = 2016-06
sbesson commented 6 years ago

Thanks for spotting this @rleigh-codelibre. It certainly feels like this could be a legacy from the decoupling work. In addition to testing the packaging as described in the PR, we might need to review which APIs or behavior have been relying on the value of this internal property.

On 8f461d6, we have not encountered this issue yet. Reading https://github.com/sphinx-doc/sphinx/issues/5417, an upcoming release seems to be planned to address this regression on old distributions. In the interim, the same workaround might need to be applied in other repositories /cc @joshmoore @jburel

rleigh-codelibre commented 6 years ago

I'm not sure there are any current users of the schema.version property, though there might have been in the past in bioformats? I thought this was the means by which bioformats would query the model version in use, but maybe we never got around to enabling that after adding the property? It's effectively public API which other people could conceivably be using though. Clearly if no one has reported it before, it's not likely to be used in any significant capacity, but the fix is pretty trivial to make it work as intended.

snoopycrimecop commented 6 years ago

Conflicting PR. Removed from build OME-FILES-CPP-DEV-merge-push-superbuild#1057. See the console output for more details. Possible conflicts:

--conflicts Conflict resolved in build BIOFORMATS-push#346. See the console output for more details.

sbesson commented 6 years ago

I have been looking for the history of this property. It looks like it was introduced in https://github.com/qidane/bioformats/pull/7 and implemented in ome.xml.model.MapPairs which was removed in https://github.com/openmicroscopy/bioformats/pull/2427. Apart from this single usage, it looks like the schema version is hardcoded in our stack although with this change, it could conceivably be read dynamically from the properties file.

The proposed change looks straightforward. I will exclude #65 which conflicts with this PR and that no-one has had the capacity to review so that this can get validated by the daily CI builds and merged.

Update (2018-09-23): following the release of Sphinx 1.8.1, I force pushed 8f461d6 away restricting this PR to the packaging changes only

rleigh-codelibre commented 6 years ago

I'll correct #65 once this is merged.