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

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

Merged
merged 4 commits into from
Apr 21, 2021

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Apr 5, 2021

Fixes #413

As I said in #413 (comment), 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
Copy link
Member

asterite commented Apr 5, 2021

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

@oprypin
Copy link
Member Author

oprypin commented Apr 5, 2021

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
Copy link
Member

straight-shoota commented Apr 5, 2021

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
Copy link
Member Author

oprypin commented Apr 5, 2021

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
Copy link
Member Author

oprypin commented Apr 5, 2021

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
Copy link
Member

straight-shoota commented Apr 5, 2021

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
Copy link
Member Author

oprypin commented Apr 5, 2021

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
Copy link
Member

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
Copy link
Member Author

oprypin commented Apr 5, 2021

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
Copy link
Member

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
Copy link
Contributor

ysbaddaden commented Apr 8, 2021

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
Copy link

hugopl commented Apr 9, 2021

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
Copy link
Member

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
Copy link
Member Author

oprypin commented Apr 10, 2021

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

oprypin commented Apr 11, 2021

@asterite I have said most of this before, mainly #413 (comment).

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
Copy link
Member

waj commented Apr 11, 2021

@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
Copy link
Member Author

oprypin commented Apr 11, 2021

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
Copy link
Member

waj commented Apr 11, 2021

@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
Copy link
Member Author

oprypin commented Apr 11, 2021

@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
Copy link
Member Author

oprypin commented Apr 11, 2021

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.

@asterite
Copy link
Member

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
Copy link
Member Author

oprypin commented Apr 11, 2021

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
Copy link
Contributor

luislavena commented Apr 11, 2021

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: luislavena/radix#32. The last time Radix had to deal with compiler changes to be compatible was in 2016 with Crystal 0.15...: luislavena/radix#8

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

❤️ ❤️ ❤️

@straight-shoota
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member Author

oprypin commented Apr 13, 2021

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
Copy link
Member

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
Copy link
Member Author

oprypin commented Apr 13, 2021

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
Copy link
Member

@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
Copy link
Member

beta-ziliani commented Apr 14, 2021

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
Copy link
Member

jhass commented Apr 14, 2021

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
Copy link
Member

straight-shoota commented Apr 14, 2021

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
Copy link
Member

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
Copy link
Member

beta-ziliani commented Apr 14, 2021

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
Copy link
Member Author

oprypin commented Apr 14, 2021

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

@beta-ziliani
Copy link
Member

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
Copy link
Member

beta-ziliani commented Apr 14, 2021

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
Copy link
Member

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
Copy link
Contributor

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

Thank you.

@beta-ziliani
Copy link
Member

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

@beta-ziliani beta-ziliani merged commit afca822 into crystal-lang:master Apr 21, 2021
@beta-ziliani
Copy link
Member

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

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.

Due to the default of crystal: <1.0.0, can't install shards for Crystal nightlies
10 participants