search

crystal-lang / shards

Dependency manager for the Crystal language
Other
765 stars 102 forks source link

Troublesome `version` values in `shard.yml` #385

Open straight-shoota opened 4 years ago

straight-shoota commented 4 years ago

Currently, shards implementation accepts just any value for the version property defined in shard.yml. The specification is not very restrictive either, so you can technically write a lot of stuff in there that doesn't make sense.

This value is mostly informational and in many use cases it doesn't matter much what it is or whether it fits a schema.

But it is also a source for the reported version of a shard when not discovered through a tag. For example when there are no tags shards uses HEAD by default. A HEAD ref is internally represented as version with commit metadata:

https://github.com/crystal-lang/shards/blob/ee74934aec8653490629c2073e8a46054bb28ffd/src/resolvers/git.cr#L157

Shards::Version doesn't validate its value either, so whatever spec.yml reports ends up there and weird values lead to an invalid instance that can't be used.

For example, cocol-project/ccl-pow at HEAD reports its version as version: v0.1.3 (the v shouldn't be there) and installing that shard fails because the internal Shards::Version can't be resolved as a valid version:

dependencies:
   ccl-pow:
     github: cocol-project/ccl-pow
$ shards --version
Shards 0.10.0 [ee74934] (2020-05-06)
$ shards install
Resolving dependencies
Fetching https://github.com/cocol-project/ccl-pow.git
Invalid version for git resolver: v0.1.3+git.commit.cee2b7994be4f780b8fe505d50e0587f39173596

For this case, it would be an option to fix this simply by stripping a leading v in the version property. Maybe that would be a nice convenience feature. But it really shouldn't be necessary (and this mistake seems to be pretty rare). The value could actually be anything, which might be allowed or not. But shards should be able to handle this situation. And in fact the latest release (0.10.0) works fine installing this shard. What's broken is the mechanism to turn a non-tag git ref into a Shards::Version with appended metadata and then expect that version to match the git tag format. Maybe an easy solution would be to strip the leading part of VERSION_AT_GIT_COMMIT and match /(.+)\+git\.commit\.([0-9a-f]+)$/. That should solve this issue, but I'm not sure if it has other implications.

Btw. Porting shardbox to use shards master already pays off as it discovers these issues :D

waj commented 4 years ago

Changing the regex definitely fixes the issue, but for example, if the HEAD is now tagged with v0.1.3 Shards will raise the warning about a mismatch between the tag and the version in the spec.

I think we should make the spec a little bit more restrictive. Even though we could fix this bug, it probably doesn't remove too much value if we say the version must be something close to a semver. Maybe semver, but with a variable amount of numeric components. It seems that is what is actually assumed in most part of the code, as there isn't any example or spec. Version comparison doesn't work either: v0.1.2 is greater than 0.1.1 but also greater than 0.1.3. Supporting just any version string would make the code much more complicated.

Something missing from my latest refactors is have a more rigid structure for the Version. If we make it more rigid, we could model it as a struct with core : Array(Int32), prerelease : String?, metadata : String? for example. That would make it easier to compare versions, without having to parse the string over and over again, and forget about assumptions and regex matching the value. In other words, less bugs.

straight-shoota commented 4 years ago

if the HEAD is now tagged with v0.1.3 Shards will raise the warning about a mismatch between the tag and the version in the spec.

Sure, in a tagged release, the version in shard.yml should match the tag version.

Non-tagged versions don't necessarily need to support the concept of comparable version identifiers.

For example in an experimental branch, I can totally make sense of calling the version in that branch experimental like the branch and not some arbitrary version number that doesn't relate to the state of the branch anyway.

IMO the behaviour of 0.10.0 is fine in this regard. I'd like it to be that way again.

waj commented 4 years ago

I don't think the author of that shard is thinking the current version is something temporary until the release. It's better to warn about the potential issue before the release.

Non-tagged versions don't necessarily need to support the concept of comparable version identifiers.

This is not true. Even for untagged identifiers, the version is used to check if the resolved version matches the requirements of other shards. The experimental flag is perfect for the prerelease portion of the version.