crystal-lang / shards

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

Don't default the Crystal version to "<1.0.0", use only the lower bound #493

Closed oprypin closed 3 years ago

oprypin commented 3 years ago

Fixes #413

As I said in https://github.com/crystal-lang/shards/issues/413#issuecomment-727671407, when discussing this breaking check, there was no motivation specified in terms of how this makes developers' lives better at all. And seems that the majority does not want this check.

Behavior before #395: crystal: field does nothing

Behavior now: crystal unspecified means < 1.0.0 crystal: x.y.z means >= x.y.z, <(x+1).0

Behavior with this PR: crystal unspecified means * / does mostly nothing crystal: x.y.z means >= x.y.z

It is still possible to explicitly write crystal: ">=0.35, <2.0" and the check will trigger.

asterite commented 3 years ago

I personally think that the version in the crystal property should just be informative, never used as a restriction.

oprypin commented 3 years ago

I cannot support that comment, because:

  1. Even if it's true, this PR solves almost all problems and is a much smaller change; the rest can be discussed separately.
  2. I like minimum version restrictions. For example, Halite made a change that works only with 1.0 and explicitly declared that, and the outcome is either an obscure compilation error (which was what I saw, because of course I run with --ignore-crystal-version...), or a message that you need to update Crystal.
straight-shoota commented 3 years ago

I'm not sure I agree on "x.y.z means >= x.y.z". That doesn't seem very useful to me. I'd rather remove x.y.z entirely instead of being a less explicit alias to >= x.y.z. Applying this change just increases confusion: it looks like = x.y.z, used to be >= x.y.z without any effect, then it was similar to ~> x.y.z, and then works as >= x.y.z.

The major confusion comes from the semantics of the absence of the crystal property. That's the core of #413. Let's focus on that.

oprypin commented 3 years ago

rather remove x.y.z entirely

Well you can't literally just "remove" it from the universe, you still need to specify what happens if people write x.y.z. What will it be?

oprypin commented 3 years ago

To clarify: that was the typical old style syntax, and there are still plenty of such lines in the wild, those are usually the ones that break due to the implied <1.0.0

straight-shoota commented 3 years ago

Of course. I was speaking purely theoretical. In practical terms, I would keep the current semantics.

Yes, the original meaning of crystal: x.y.z was >= x.y.z, but it did not have any effect. Now it is similar to ~> x.y.z and developers have adopted that. This change had an active effect long before the issues with 1.0 upper bound. I think I've come to accept these semantics. It's not bad, actually. Maybe not very useful either, but I don't see enough motivation to change it.

Anyways, I'd like to separate concerns and focus on the absence of the crystal property here, to hopefully get this merged. The behaviour of a plain version number is discussed in #449 and can be changed in a separate PR. Talking about two separate changes in this PR makes the process unnecessarily hard.

oprypin commented 3 years ago

focus on the absence of the crystal property

Why? I think you may be misunderstanding the scope of the issue. Libraries currently fail to install both when the Crystal version is unspecified and when it's specified as 0.x.x. With the latter being the majority, I think. <1.0.0 is implied in both scenarios, though for slightly different reasons.

straight-shoota commented 3 years ago

I just don't think that "x.y.z means >= x.y.z" is good by itself. To express that, you should just write >= x.y.z explicitly.

For me, the only benefit of that change is to mitigate installation failures of outdated dependencies that still depend on the old crystal version semantics.

That might have been a reason to include this change in the shards version distributed with Crystal 1.0. But I think it's too late for that now. A new shards release will only have an effect when we release Crystal 1.1. By then, I'd assume there wouldn't be much damage prevention left to warrant the change and create more confusion.

oprypin commented 3 years ago

I just don't think that "x.y.z means >= x.y.z" is good by itself. To express that, you should just write >= x.y.z explicitly.

Sure. I even agree with that. But we are dealing with numerous libraries that already have that specified, so we can't just say "it's not good" and hide.

For me, the only benefit of that change is to mitigate installation failures of outdated dependencies that still depend on the old crystal version semantics.

Well yes, isn't that what we're doing here? That's 100% of my motivation.

But I think it's too late for that now.

Alright, guess I'll just close this.

straight-shoota commented 3 years ago

Well, I don't think that part is worth it. But we can still come to a different conclusion. And changing the default when crystal property is absent is good on its own.

ysbaddaden commented 3 years ago

I second that. I'm bored with pull requests for empty releases that are a mere "identical to previous version, but installable with crystal 1.0". My shards were already compatible with 1.0. I'm fine with fixing bugs.

These shards are fully compatible with Crystal 1.0, but Shards won't install them:

I'll have a bunch more among the next weeks.

hugopl commented 3 years ago

One common thing is when your shard is ok but depends on shards that aren't. What could help would be let shards explicit declare that it should ignore crystal versions of some dependencies, e.g.:

dependencies:
  cute:
    github: Papierkorb/cute
    ignore_crystal_version: true

And I agree with the MR.... but in fact it come too late :-(. Have the upper bound is useless since nobody can predict the future to say that a shard wont work with some version, so better to guarantee just what's know at the time, the lower bound crystal version supported.

bcardiff commented 3 years ago

Although I understand the present struggle while 1.0 is young, I think that explicit dependencies with versions are a necessity in the long run if you want to allow things to keep working and being usable in scenarios where you can't or wont use the latest version of everything.

I fail to see why the language & std-lib version should be different than depending on libraries.

Having informative versions offers no clue whether newer or older version should work. If offers no contract whether the author is willing nor was expecting for other than the informative version to support.

If I publish a package for 0.x and I am no longer interested in updating it support 1.0 that is fine. The community will need to use a fork that will be maintained. If not, overrides offers a workaround.

Of course, things can change from the current status, but I believe the current one is better for the long run.

oprypin commented 3 years ago

If I publish a package for 0.x and I am no longer interested in updating it support 1.0

That is exactly how you make people uninterested. See @ysbaddaden above.

What the current state achieves:

  1. Users internalize that Shards is finicky and just needs that flag for whatever reason.
  2. Developers are annoyed and (hopefully) change from 0.35.0 to >= 0.35.0 and the deps are back to the same (unpinned) situation.

And um let's not even put it as "people who are no longer interested", with this in mind:

https://github.com/crystal-lang/html_builder/blob/master/shard.yml https://github.com/crystal-lang/crystal-random/blob/master/shard.yml https://github.com/crystal-lang/crystal-env/blob/master/shard.yml https://github.com/crystal-lang/crystal-readline/blob/master/shard.yml

(there were even more, but were only very recently updated)

asterite commented 3 years ago

If I wrote a package that worked fine for 0.35.1 and it still compiles fine for 1.0.0, why do I need to release a new version with no changes?

asterite commented 3 years ago

https://twitter.com/ysbaddaden/status/1381015518728089603?s=20

Can we please fix it? This doesn't make any sense.

Say I have a shard that says crystal: 0.35.1. Now crystal 1.0.0 is out with some changes. Look at this:

Before this PR After this PR
if my shard still works fine with 1.0 error: incompatible version it works!
if my shard doesn't work fine with 1.0 error: compile error error: compile error

As you can see, before this PR we simply ban any shard that wasn't updated to say "crystal: 1.0.0" , "crystal >= 0.35.1", etc. But with this PR, in the happy path scenario (the shard still works fine with 1.0) things just work fine out of the box. Users are not annoyed.

In the case it doesn't work fine you will get a compile error. You'll have to make changes to the shard. That's also true before this PR! So the situation remains the same for broken shards. But for working shards, the situation is improved.

There's a catch: what if crystal 1.0.0 introduced a semantic change? Then maybe the shard still compiles fine with 1.0, but it works in a bad way. Should we just consider all shards broken because of that? NO! If there's a semantic issue, you'll surely find out when you install the shard. Because maybe my shard has a semantic bug now in 1.0, but what if I don't have a test to prove it?

In summary, knowing if something works fine with 1.0 besides compile errors is impossible to automate, so we shouldn't just ban all existing shards.

Please, please, change this! It gives a really bad image of Crystal 🙏

Another summary: if my shards says crystal: x.y.z it should be assumed that from that the shard will work fine for any crystal version following that. That is, the crystal version requires means "I need at least crystal version x.y.z" because maybe in that version a new feature was introduced. Let's assume it's very unlikely we'll break people's code right now. Ruby has been doing that for years. When a new Ruby version is released, nobody has to go and update their gems to say "Oh, and now this works with Ruby 3". As people write more and more shards, shards that work fine and will never need an update, this will become a bigger problem.

Look at this shard: https://github.com/maiha/base32.cr/blob/main/shard.yml . It's really simple! Just uses a couple of String, Int and Hash functions. But guess what? No crystal version is specified. Nobody can install it without passing --ignore-crystal-version (I just tried it.) How can we fix this? I have to send a PR saying "Please specify a crystal version". A new version is released. A lot of time wasted, because the shard works just fine in any crystal version, and it will continue to work fine forever.

asterite commented 3 years ago

What's even worse is that there's an escape hatch: I can actually install that shard if I pass --ignore-crystal-version. What does that mean? The next developer that tries to install that shard will get the same error message. One more person annoyed! And they can continue ignoring this error, and so on.

If there's the possibility to pass --ignore-crystal-version, what's the point? You want to make sure a shard is guaranteed to work fine with 1.0.0 (practically impossible!), but then you provide a workaround so that people can still use them without that guarantee.

oprypin commented 3 years ago

@asterite I have said most of this before, mainly https://github.com/crystal-lang/shards/issues/413#issuecomment-727671407.

Absolutely all of this was predicted, we didn't need to see it to realize. Moreover, almost everyone agreed that this was not good, including the majority of core team members. The fact that a few people were still able to push this through, without listing any real advantages of this, and ignoring the numerous pointed out disadvantages, should be (for lack of a better word) investigated.

Even now it is really weird that anyone needs to plead for this, especially @asterite, the creator of the language, and with overwhelming support. What or who are we waiting for?

waj commented 3 years ago

@asterite having the author of the shard making a new release just to change the version is nothing compared with everyone else having to wonder if a shard is still compatible with new Crystal versions or not. It's annoying to have to release just one more version of each shard just to change the Crystal version in it? Sure. But the idea was to remove that annoyance from everyone else for unmaintained shards. Setting the version doesn't give any warranty, but at least is the author who claims its compatibility. Authors that set this value to * are just not caring at all for the users once future (incompatible) versions of Crystal are released. Did it work without changes? nice! good for the maintainer. But that's not the experience for anyone else. And it's not the transition from 0.36 -> 1.0 we're talking here. It's future changes from 1.x -> 2.x that might be more drastic that we'd like to get covered. Shards with crystal: * in it will very likely be broken in 2.0.

So, the idea was to give a tool to help the community, not to give bad image. Actually I think it's the attitude of many main contributors what hurts the image.

oprypin commented 3 years ago

having to wonder if a shard is still compatible with new Crystal versions or not

...like they're doing now?

And it's not the transition from 0.36 -> 1.0 we're talking here

Yes, that is actually what we are talking here. The transition is broken and let's not bring far future into this.

waj commented 3 years ago

@oprypin please don't quote just part of what I said to take them out of context. That's disrespectful. And the transition is broken just because shards maintainers have to release a new version that only takes 5 minutes to them to do it?

What I'm saying is that we were more concerned about future migrations from 1.x to 2.x than the migration from 0.36 to 1.0. We wanted, as I think we stated many times, to be able to make non backward compatible changes in 2.x.

oprypin commented 3 years ago

@waj I disagree that that quote is any different without context than in context.

And let me reiterate why I disagree with it. People here are talking exclusively about the ongoing transition to 1.0, and then you come in and deflect that by talking about long-term considerations.

The long-term considerations cannot be used as an argument regarding the current state, because there's literally no way for them to kick in until Crystal 2.0 is on the horizon.

And the transition is broken just because shards maintainers have to release a new version that only takes 5 minutes to them to do it?

Yes.

oprypin commented 3 years ago

And let me make the agreeable solution fully clear:

Removing the check that says 0.36.0 and 1.0 are incompatible now doesn't mean that later 1.0 and 2.0 can't be declared incompatible.

waj commented 3 years ago

Removing the check that says 0.36.0 and 1.0 are incompatible now doesn't mean that later 1.0 and 2.0 can't be declared incompatible.

But that's not what I understand from this PR. This is PR is making shards that now have a crystal: 0.x or no Crystal version at all, compatible with any future version of Crystal, right?

Of course we knew that all shards would’ve to be re-released for 1.0, but that was a conscious decision. A small price to pay now to have a cleaner ecosystem later. For some reason seems like this is too problematic. I don't share that viewpoint although I understand that many would’ve preferred not having to release anything else at all. But saying that the transition is broken just because 5 minutes are needed to solve it I think it's overreacting to the situation.

I'd love to see a solution for that problem, but one that doesn't completely disregard the problem that @bcardiff and me wanted to solve. Otherwise this conversation will be forever in conflict, and I'm not happy with that.

Should we make shards released for 0.x automatically accepted for 1.x? Maybe that's a compromise point we could discuss.

oprypin commented 3 years ago

This is PR is making shards that now have a crystal: 0.x or no Crystal version at all, compatible with any future version of Crystal, right?

Right. Returning to a good known state.

I'd love to see a solution for that problem, but one that doesn't completely disregard the problem that @bcardiff and me wanted to solve.

As I said, merging this pull request is not a statement that we must never declare 1.0 incompatible with 2.0. So this is not disregarded, but only postponed.

We don't need to fully decide on and implement the distinction between (x.y->1.0) and (x.y->2.0) right now.

Otherwise this conversation will be forever in conflict, and I'm not happy with that.

If the situation will be forever in conflict, better keep the code's behavior in a known good state rather than a known bad state.

Of course we knew that all shards would’ve to be re-released for 1.0, but that was a conscious decision.

I don't disagree that the decision was conscious. I am just saying that it was wrong and also did not follow consensus.

asterite commented 3 years ago

But the idea was to remove that annoyance from everyone else for unmaintained shards

Just because a shard is unmaintained doesn't mean it won't work in future Crystal versions. Consider the base32 example above. It will always work fine. Or maybe it won't work in Crystal 2.0? In that case they will submit a fix and release a new version. But now you are forcing everyone to release a new version, even if no fix is needed.

I can also release a shard that says "crystal: 1.0.0" and when you install it it will always give a compile error. Just becausing something is marked to be compatible with 1.0.0 doesn't mean it will work with that version.

asterite commented 3 years ago

So this is not disregarded, but only postponed

I think this is the key point. Why prepare for the up coming 2.0 which we still have no idea how (or if) it will break things? Not much changed between 0.35.1 and 1.0.0, so the majority of shards will work just fine.

What other language does a restriction like this at the language level? And why isn't this a problem in languages that don't do this?

oprypin commented 3 years ago

What other language does a restriction like this at the language level? And why isn't this a problem in languages that don't do this?

Please let's not delve into this, it doesn't matter at the moment.

I am only reverting the biggest breaking change for syntax which was used everywhere but never had such a meaning.

And the meaning of the syntax doesn't even matter much for new usages because almost everyone uses >= 0.35.0 etc anyway. So it mostly only hurts old usages. And it's even possible to still use this suggested system, by writing ~> 1.0 or >= 1.0, < 2.0.

luislavena commented 3 years ago

As other shards authors, I was hit by this empty update to keep compatibility.

As Crystal user, I was extremely annoyed that I had to fork most of my dependencies and correct the Crystal version, even that all these shards were already compatible with 1.0.0

Coming from Ruby land, as RubyGems contributor and author of some gems, I can comment that is an utopia trying to force and coerce a required Ruby version to library authors and that ends causing is lot of noise and frustration on users.

A library author cannot predict if next minor or major version of your language will break the specific parts of the language your library is using. Only nearing a release/nightly of what is going to be is the only moment you can determine if you need to reduce your supported spectrum of compatible versions. But that also means your library is maintained.

We all dream with semantic versioning will be the solution, but even that while try it, we know that sometimes that is not possible.

There are libraries that are feature complete, don't need updates, but this new version restriction is adding noise to someone that already created and shipped something that already works, that doesn't depend on anything that could break.

One concrete example, but many similar PR: https://github.com/luislavena/radix/pull/32. The last time Radix had to deal with compiler changes to be compatible was in 2016 with Crystal 0.15...: https://github.com/luislavena/radix/pull/8

Thanks for reverting this change and hope can be part of the next release.

❤️ ❤️ ❤️

straight-shoota commented 3 years ago

@waj

But saying that the transition is broken just because 5 minutes are needed to solve it I think it's overreacting to the situation.

I think this completely misses the real problem here. The 5 minutes every shard maintainer needs to update the shard is negligible. If that just "somehow" happend around release time, everything would be perfect. But it is not. It takes time for maintainers to realize they need to act and be able to act. By then, countless users have already faced problems with failing shard installs. Figuring out why shards install breaks, asking maintainers to release and waiting for the release amounts to many minutes of collective time spent dealing with this. It effectively steals hours of time from shard users trying to update their Crystal code to the new release. That's not a small price. It's a huge price. It drives people off when things are just broken.

The fact that even many crystal-lang shards are still missing a release to declare compatibility with Crystal 1.0 is a clear confession of failure.

bcardiff commented 3 years ago

I want to revisit more or less the criteria used.

Ideally, I would have prefered that the same semantics could be applied for crystal version and for any other dependencies. Easier to explain and tidy enough.

But that would have broken all packages and crystal init would need to be updated since crystal: x.y.z would mean = x.y.z. Since shards does not enforce semver ¬¬ the = is the only valid choice.

Prior Shards 0.14 the crystal version was informative. If we would have kept that then there was two choices: Add a schema version to the shard.yml since the semantic was changing or make a special rule for crystal: 0.y.z that would been translated to crystal: * since that's what it was.

Both alternatives were more uncomfortable to explain to the user. So the best we could do, in order to have a strict version check, and not break the ecosystem for Crystal 0.x was to make crystal: x.y.z equals to ~> x.y, >= x.y.z. Note that Crystal 0.x is not broken.

The decision was made consiously and with Crystal 2.0 in mind. I don't think is a good thing to not establish a policy for 2.0 and see it in the future.


I think that the price of updating the ecosystem on major versions is fine. Shards might not be migrated, it's fine, packages are always being deprecated.

The fact the some people are complaining is because they don't agree or don't know about this change, despite that we've announced it in Aug '20. And the fact that some crystal-lang packages have not been updated is because we lack organization or we think maybe they are worth leaving them behind in the 0.x era.

Things are not broken, but some prefer a more loose semantics in Crystal version. I wonder, why that is not try on every other package? Why not leave just the package name and let it be? Don't you think that will be to chaotic in the long run?

Making the crystal: 0.x.y packages available on 1.x can be made, as stated, with a special rule. Make crystal: 0.x.y same as crystal: *. But this is allowing us to not pay the price because the last 0.x is almost 1.0. The same might be wanted for 1.x to 2.0 migration because we will be used to that, so what? another special rule?. Note that even if 0.x.y is *, then x.y.z could still be ~> x.y, >= x.y.z. It depends on the long run policy to adopt.

If we don't honor semver in the crystal version restriction then we are left probably with what this PR is aiming: crystal: x.y.z to be >= x.y.z. But to me, that means that the compiler and std-lib will not be obliged to honor semver either, not because of >= clearly, but because of how it is wanted to be presented "good luck, it may work". It sounds like a slipper slope.

I don't think we will convince each other. It's a matter of preferences, there is no right or wrong. The most I can do is to remove myself from the equation regarding this topic.

ysbaddaden commented 3 years ago

@bcardiff My problem is the default constraint being < 1.0.0 when shards don't specify a crystal constraint. This is unexpected and annoying. I'm fine fixing the constraint when it exists to >= or ~> or whatever other constraint I'll choose (including none) not what shards decided to —wrongly in too many cases.

beta-ziliani commented 3 years ago

Sorry to cheap in late in the conversation, I'm just detaching the parachute from my back... It's clear that this is a 7-headed beast, and I don't think there is an obvious "right" or "wrong" on this matter.

Now, as I see it, the >= x.y.z behavior is strange. It sounds more like "hey, I tested it on x.y.z and I hope it will continue to work in the future. Cross your fingers!". Then, what's the point? We can also try compiling on previous versions and try our luck, don't we? This is also true even with the actual behavior of < (x+1).0.

And let me note something: even a small thing like adding a type annotation might break a dependency. Then, even claiming "x.y" might be a lie when transitioning from "x.y.z" to "x.y.(z+1)". And, like Ary mentioned above, there can be a semantic difference breaking things...

So we have two options: If we ignore the check, users have to find out if things work in their systems or not. If we impose a restriction (=, >=, whatever), if it is too restrictive it will force many people to --ignore it, and if its too bland it will fail to compile in many computers even when expected to.

Then, what is the way out? My proposal is to default to warning: "The maintainer didn't specify it works on your system; they might be pleased to know how it went. Note that it might compile, yet not work properly.", with a --strict version (mostly for CI purposes) turning the warning to error. This way, the field will list all the versions/ranges in which it is known to work, and it's up to the maintainers to keep it up to date or to have all users be thrown the warning at their faces.

oprypin commented 3 years ago

This appears to be stalled. A "perfect" solution was suggested, but is anyone working on the implementation, and is it going to be approved any time soon? The bleeding is ongoing. Why is it so difficult to just revert the check for now, release, and then proceed to add it back in whatever shape desired?

beta-ziliani commented 3 years ago

The "bleeding" will continue until the next release is out, isn't it? Just merging this PR won't make a release. Or am I missing something?

oprypin commented 3 years ago

Right, so a release should be made after merging this, and the apt repos can get a crystal-1.0.0-2 package with this. (really shards should be a separate package but this is not the time to change that)

bcardiff commented 3 years ago

@beta-ziliani suggestion is implemented in https://github.com/crystal-lang/shards/tree/feature/warn-only-crystal-version and is based on this PR in case that is wanted.

What is the expected semantics of crystal: x.y.z is still on discussion it seems, but is independent of my proposed changes.

beta-ziliani commented 3 years ago

About crystal: x.y.z: I think that the obvious thing with the warning mode in place is to consider it =x.y.z. Someone believes not?

jhass commented 3 years ago

Given the current behavior I'm not super confident that most people have put a restriction specifier into their shard.ymls , vs just the last tested version. Before deciding this I'd recommend a small survey on the ecosystem on how the version system is used, that is percentage of shards using version: x.y.z, version: >= x.y.z, version: ~> x.y or something else respectively.

straight-shoota commented 3 years ago

Statistics for shards listed at shardbox are available here: https://shardbox.org/stats

A more condensed summary:

crystal property Amount Percentage
version restriction (x.y.z) 568 51%
none 332 30%
range restriction (>= x.y.z, ~> x.y.z etc.) 212 19%
asterite commented 3 years ago

Given that, I think crystal: x.y.z should mean crystal: >= x.y.z. I think more or less what this PR proposes.

beta-ziliani commented 3 years ago

Perhaps there was something of my proposal that got lost in communication, or I missed something about all this (in which case, be patient and explain it like I'm 5...). A better way of understanding my idea is as a "compatibility" badge, and this is something we can't possibly put in the future. By not committing to the future, we only say a honest "we don't know" to those that are not listed in the field. What is the problem with this, and how would the commitment to the future fix it?

oprypin commented 3 years ago

@beta-ziliani Honestly now I'm actually confused, whereas after your previous message (or I mean 3 ago) I wasn't.

beta-ziliani commented 3 years ago

Re-reading my message I see it's pretty obvious I'm against promising things will work in the future. Isn't it? I'm confused too!

beta-ziliani commented 3 years ago

But it doesn't really matter how we interpret the lack of symbol in front of the version no. I was aiming to the more restrictive interpretation.

beta-ziliani commented 3 years ago

I just made the PR form @bcardiff code of my proposal (+ this PR). If this is sufficiently good for everyone, I say let's move on :-)

luislavena commented 3 years ago

Hello @bcardiff @beta-ziliani, any further updates on #496 that are blocking?

Thank you.

beta-ziliani commented 3 years ago

We'll get it soon, thanks for pinging @luislavena

beta-ziliani commented 3 years ago

496 was merged including this PR, I'm closing this one.