jessedoyle / duktape.cr

Evaluate JavaScript from Crystal!
MIT License
137 stars 17 forks source link

update to crystal version 1.0.0 #64

Closed Kanezoh closed 3 years ago

Kanezoh commented 3 years ago

Thank you for this shard! This is what I wanted, but when I tried to install this shard on crystal version 1.0.0, crystal version is fixed under 1.0.0 and failed. Here is the errors.

Unable to satisfy the following requirements:

- `crystal (< 1.0.0)` required by `duktape 0.21.0`
Failed to resolve dependencies, try updating incompatible shards or use --ignore-crystal-version as a workaround if no update is available.

So, I specify crystal version as over 0.35.1 in shard.yml.

Thank you.

jessedoyle commented 3 years ago

Hi @Kanezoh - thanks for pull request!

I'm pretty sure we don't specify the required crystal version anywhere, so shards shouldn't be failing on that constraint. I'll have to do a bit of digging, but I've installed the current version of duktape locally with Crystal 1.0.0 without issues.

Let's do a bit of investigation first, I'd like to avoid specifying any constraints on the Crystal version in shard.yml if possible.

Kanezoh commented 3 years ago

Thank you for replying @jessedoyle!

I also think that the conflict is curious cause there are no crystal version specifications in shard.yml file. Now, I'm investigating the cause, but I found this in shard.

https://github.com/crystal-lang/shards/blob/addc26a3f22fee9fccb01cbb3e8878bef02d4295/src/molinillo_solver.cr#L184

if there are no crystal version specifications, the crystal version is automatically set under 1.0.0. So, duktape's crystal requirement is set as so.

But I don't know why it's working in your local environment...

Kanezoh commented 3 years ago

FYI, I found the related issue. https://github.com/crystal-lang/shards/issues/413 If you want to cover all the versions, specifying >=0.1.0 makes sense, but I don't think that supporting all the old versions is a good idea.

jessedoyle commented 3 years ago

@Kanezoh - You're right, thanks for the link!

I'm surprised on the design decision by the Crystal team to default all shards without an explicit crystal constraint to < 1.0.0. That decision really doesn't make a ton of sense in my opinion.

With that being said your PR looks great! We can merge this shortly. One small thing is that we should add a brief changelog description and probably cut a 1.0.0 release as the core language is now 1.0.

If you're okay with it, I can push a commit to your branch that adds some small tweaks. Does that sound good?

Kanezoh commented 3 years ago

Thank you for merging! I also think that current behavior of shards is troublesome...😌 Anyway, using this shard in Crystal 1.0.0 is what I wanted! thx!