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

Use crystal property to filter shard compatibility #395

Merged
merged 5 commits into from
Jun 3, 2020

Conversation

waj
Copy link
Member

@waj waj commented May 22, 2020

This PR implements what's discussed in #365

I had to also make the assumption that when the crystal property is not present, the expression < 1.0.0 is assumed. This is because old shards in the past didn't specify the crystal version. If we don't make this assumption, once Crystal 1.0.0 is released, old shard versions will be installed by default. This has the implication that the parameter is now mandatory after 1.0.0 but I think that's a good thing and according to what was discussed.

@straight-shoota
Copy link
Member

I don't follow. The discussion in #365 was about redefining the semantics of crystal property. Making it essentially mandatory was not part of that discussion. And I don't see a reason for that.
It's supposed to be a restriction. If not present, it means there's no restriction. So it should be an implicit crystal: "*".

If we don't make this assumption, once Crystal 1.0.0 is released, old shard versions will be installed by default.

Why would old versions be installed? Shards should always pick the newest version that's compatible.

@@ -150,7 +151,20 @@ module Shards
end

def dependencies_for(specification : S) : Array(R)
specification.dependencies
return specification.dependencies if specification.name == "crystal"
crystal_pattern =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so neat 😻

@bcardiff
Copy link
Member

Why would old versions be installed? Shards should always pick the newest version that's compatible.

if my_shard 0.1.0 is published today without crystal restriction,
then is published as 0.2.0 with crystal: 1.1.0 restriction,
when crystal 2.0 is out the compatible version will be odly 0.1.0.

I think is better to force a < 1.0.0 as default restriction. It will be no worse than in the next major release. Whether that mean changing the source code or just bumping the crystal restriction.

Having a default unrestricted crystal version seems as a bad practice as pointing to a shard repo without version restriction.

@waj
Copy link
Member Author

waj commented May 22, 2020

Why would old versions be installed? Shards should always pick the newest version that's compatible.

Consider this scenario: a shard today published to be compatible with 0.x was published in the past without the constraint (because it didn't exist). Once Crystal 1.0 is released, if you try to install that shard, an old version will be installed, instead of just raising an error because the shard was not updated.

A desirable side effect of this is that unmaintained shards clear themselves from the ecosystem. This is the approach that Elm uses and it works really well. Making the crystal version optional allows (already) old versions of shards to last forever.

On the other hand, I don't see any advantage on keeping the property optional. Is it possible to write a shard that is compatible with any Crystal version in the future? Anyway, if someone want to do that is still possible by writing crystal: >= 0 but I'd prefer to see that explicit.

@straight-shoota
Copy link
Member

This hasn't been a major problem so far, with constantly breaking Crystal versions. Why would it be a problem in the future?

I don't know any dependency managers that make such a restrictive implication about language version compatibility.

This change makes all shards incompatible with Crystal 1.0. Every shard would need to make a new release, even if nothing has changed (and it may have actually been compatible with Crystal 1.0 for the last X releases).

Even when we bracket the semantics of the property being absent, 60% of all shards currently use this property according to the old semantics, i.e. optional indication of minimal compatible version. All these shards would no longer install with this change to dependency resolution.

Such a huge disruption is completely unacceptable. It breaks the entire ecosystem.

The only action we can take right now is to update the specification to describe the intended semantics. But it can't have an immediate effect as long as the vast majority of shards is still on the old semantics. And even the new semantics seem to need some discussion about the meaning of absence.
The following step could be some kind of warning, possibly with a shards validate tool (#29).

@waj
Copy link
Member Author

waj commented May 22, 2020

This change makes all shards incompatible with Crystal 1.0

And that's the idea. Shards should be tested and upgraded to work with Crystal 1.0. With so many deprecations and breaking changes I doubt I could trust any shard could just work once we release 1.0.

Such a huge disruption is completely unacceptable. It breaks the entire ecosystem.

Quite the opposite. It might feel disruptive at first but in the long term it will help to keep a healthy ecosystem. Any shard owner that is still maintaining the code, once Crystal 1.0 is released just need to check for incompatibilities, change the shard.yml and tag a new version. If they were keeping the source up to date with latest changes and pay attention to deprecation notices, it should take just a couple of minutes. And it wont happen that often. Just for any major Crystal release.

I don't know any dependency managers that make such a restrictive implication about language version compatibility.

Yes, there are examples. Dart also implemented this constraint and it seems is mandatory since 2.0 (https://dart.dev/dart-2). Elm also has very strict constraints.

@straight-shoota
Copy link
Member

straight-shoota commented May 22, 2020

This might work if every shard owner updates publishes their shards immediately. But that's totally unrealistic. Even if every individual release can be done in a few minutes or seconds, the entire process is certainly going to take a long time because many shard owners won't be able to respond on short notice.
The result would be hundreds of shards that work perfectly fine on 1.0 but shards refuses to install them. Applications won't update to use Crystal 1.0 when their dependencies are not updated, and that reduces the overall pressure to update to 1.0.
Instead, I'd like everyone to be able to start using Crystal 1.0 as soon as it comes out.

As the user of a shard, I care that I can install and use that shard. I don't care whether it says it works. As long as it works, I want to be able to use it.
I want shards to do that for me and not make me fork the shard I'm depending on only to adjust the crystal property.

I'm open to discussing strict Crystal version dependencies, but there's no way this can be adapted quickly.

@RX14
Copy link
Contributor

RX14 commented May 22, 2020

I'm with @straight-shoota. I think this is impractical and will cause a lot of pain in the real world if implemented in the near-term.

Maybe we can roll this out after a lot of testing and a lot of warnings. But not now.

@bcardiff
Copy link
Member

I see that enforcing an implicit < 1.0.0 is consistent with the future default values for crystal init: crystal: x.y.z meaning crystal: ~> x.y, >= x.y.z.

The proposed implicit value < 1.0 is consistent with the current status: all shards have been targeted to 0.x version the language and std-lib so far.

Everything that is working today is doing it with a 0.x version of crystal and std-lib. It is understandable that there is a maintenance to make them available in 1.0. When discussing the upper bound constraints implications, we acknowledge that on a major release implies a revisit on published shards. Going from 0.x to 1.0 is the first of those events.

Having a crystal constraint is like having the std-lib as a dependency. How would you know which version of the std-lib you are expecting to use otherwise? When adding a shard dependency, I think it's a bad practice to leave it unconstrained.

I think it encourages a better ecosystem that packages have version constraints, for dependencies and std-lib. I see no reason to treat std-lib different than any other dependency.

I don't like to encourage an ecosystem where the dependencies are so loosly stated that you might not even know which version of the language is a package ready for.

If we think of Crystal running in the long run, not everything will need to run in the latest version. If we allow optional crystal constraint, that won't work.

As for other ecosystem examples:

  • Elm enforces minor version since they are in 0., that is more strict that what we've done so far and the ecosystem is able to move forward.
  • Haskell/cabal uses base to express this kind of dependencies and also enforce upper bounds. https://www.haskell.org/cabal/users-guide/developing-packages.html
  • Dart also enforce upper bounds, yet they didn't start from the beginning and they choose <2.0.0 as the implicit value.

We might need to revisit how pre release version of the std-lib will be interpreted, to ensure the the ecosystem is able to catch up without too much hassle. Although that story is again, not that different from how a shard can be up to date with head version of dependencies.

@jhass
Copy link
Member

jhass commented May 26, 2020

I'm fine with an implicit < 1 constraint, if it's overridable by the library consumer.

This could either be a simple flag to shards install or a per dependency override to ignore version constraints on the dependency in general, including the Crystal version.

The current proposal will almost certainly cause the Python 2 - 3 kind of schism with each major Crystal release. Unmaintained code that still works and is used in dependencies after a major Crystal release is and will remain a reality we need to face.

@RX14
Copy link
Contributor

RX14 commented May 26, 2020

The problem is that we're gonna release 1.0, end up at the top of HN, and nobody's gonna be able to use it because none of the shards will have updated. Bad experience for everyone.

Some shards are unmaintained because they're simple enough to just keep working. Forcing them to make a pointless release will bring ire for little gain. 2.0 will end up with a significant gain over 1.0, where 1.0 has no gain over 0.25.0. So you can sell that a lot more easily. This is a community dynamics problem in reality, not one to be solved by technical purity.

@waj
Copy link
Member Author

waj commented May 26, 2020

Nobody says we're going to release 1.0 overnight without giving time for shards to prepare. There's going to be 1.0 prereleases that everyone will be using to test the compatibility of their shards. It's gonna be bad experience anyway if you rush to install shards and none of them even compile because the authors didn't take the time to pay attention to deprecation notices. BTW, majority of the most used shards already have versions explicitly defined, so the "pain" would already exist. @bcardiff have been always taking the time to test many broadly used shards before each release, sending PRs in many cases to help them keep up to date with so many breaking changes.

We can enable some mechanisms to disable or circumvent dependency checking, like local overrides or force install. That also applies to dependencies between shards. But still we should provide a good path to prepare for 1.0.

I think we could make the parsing of the current Crystal version to ignore the prerelease portion. That way shards doesn't need to be re-published between 1.0-pre and 1.0. In other words, the sequence would be:

  • Crystal 1.0.0-pre1 is published
  • Shards create new releases with crystal: 1.0.0. Just like any shard release for 0.x because of incompatibilities.
  • Crystal 1.0.0 is released. All the shards already published for the prerelease are ready to go.

@waj
Copy link
Member Author

waj commented May 26, 2020

I just added another commit to relax the version comparison during prerelease as I explained in the previous comment. Before the 1.0 release we could add options to disable the check on individual shards o globally. Let's see what's the best option for that.

Once 1.0.0-pre1 is released we will be able to experiment with this behaviour. As I said, most of the most famous shards will have to make the update anyway. I don't think this is going to represent ire from the community, but let's see.

end)
end

def self.crystal_version(@@crystal_version : String)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def self.crystal_version(@@crystal_version : String)
def self.crystal_version=(@@crystal_version : String)

was this supposed to be a setter? and used somewhere?

@RX14
Copy link
Contributor

RX14 commented May 27, 2020

I'm still against merging this.

asterite
asterite previously approved these changes May 27, 2020
@asterite
Copy link
Member

asterite commented May 27, 2020

I have a quedtion: if I mark my shard as compatible with 1.0 and Crystal 2.0 is released, if I don't update my shard then it's not eligible for install anymore?

@jhass
Copy link
Member

jhass commented May 27, 2020

I have a quedtion: if I mark my shard as compatible with 1.0 and Crystal 2.0 is released, if I don't update my shard then it's not eligible for install anymore?

Yes, that's the current proposal and the issue we (@RX14, @straight-shoota and me) seem to have about this.

I'm fine with warning or even failing there, but it absolutely needs to have a bypass.

@asterite asterite dismissed their stale review May 27, 2020 11:34

nonsense

@asterite
Copy link
Member

So my experience with Elm when upgrading to 0.19.0 from 0.18.0 was that a lot of packages refused to install because they didn't say "elm: 0.19.0". But maybe those packages compile without changes? What we ended up doing is creating forks of those packages to be able to use them. The authors were maybe on vacation, maybe didn't have time to look at it anymore (even if the code still worked fine! Code doesn't degrade like fruits). It's pretty annoying, I think.

Specially with a compiled language, if you have a newer Crystal version and you try to use a package that wasn't "supposedly" updated to the newer version, you will immediately find out if the code doesn't compile. If it compiles, then there's a really big chance that it works.

Ruby has been like that for years and it's worked really well.

Also, if I request an author to upgrade their code to Crystal 2.0.0, can they go ahead, set "crystal: 2.0.0", tag, commit and push without even verifying that the code works for Crystal 2.0.0? Then the shard will be compatible according to shards, but it will fail compilation. That means the property is still a "best effort" thing.

On the other hand, if we trust authors to always have time to keep updating the crystal property (which is a bit unrealistic, because people take vacations, they have family and other stuff to do) (and also code doesn't rotten like a fruit if you don't update it) then this might be a good compromise. It could mean all shards marked with crystal: x are %99.99 likely to work with that crystal version. Then you will almost never find surprises that you install some shards but then the code doesn't compile.

I still don't know which one I prefer. Both have pros and cons.

@waj
Copy link
Member Author

waj commented May 27, 2020

@jhass I agree with the bypass, just that I prefer to do that on a separate PR. We have time because this wont have real impact until 1.0

@asterite Elm approach was quite more strict. Maybe too much. This only disables automatic compatibility between major versions. Many times we said Crystal 2.0 might have some breaking changes. Note that this also works the other way around: if someone still uses 1.0, there is no need to do any juggling with the versions to add new shards to the project, because the dependency manager already would be filtering the compatible ones.

Note also, that the restriction can be disabled or relaxed by a shard owner by specifying crystal: *, crystal: > 0 or crystal: >= 1, <3.

All this was already discussed in #365. I just changed the semantics of not having a crystal version from * to < 1.0.0 because there are many shards that didn't have the attribute in the past but they have it now. So without this change, as soon as we release 1.0.0-pre1, the older version gets selected instead of just failing because the shards was not updated yet. I can revert this change, but I think it's a small price to pay at the beginning instead of carrying with this heritage for the eternity (yeah, I have big plans for Crystal :o). Majority of well maintained shards will do the update between 1.0.0-pre1 and 1.0.0, so nobody would notice disruption on those shards by the time of release. On the other hand, abandoned shards could be left out of sight in indexers for example if you could optionally filter by Crystal version.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it!

Comment on lines +85 to +88
if version =~ /^(\d+)\.(\d+)\.(\d+)([^\w]\w+)$/
"#{$1}.#{$2}.#{$3}"
else
version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use SemanticVersion for that?

@waj waj merged commit c48d19b into crystal-lang:master Jun 3, 2020
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

Successfully merging this pull request may close these issues.

None yet

7 participants