racket / pkg-index

7 stars 14 forks source link

Deprecated dependency syntax is no longer accepted #15

Open samth opened 6 years ago

samth commented 6 years ago

This package errors currently: https://pkgs.racket-lang.org/package/gaming

However, the package spec seems to fit the deprecated form described here: http://docs.racket-lang.org/pkg/metadata.html?q=pkg

Error message:

pkg: invalid `deps' specification
  specification: '(("ramunk" "1.0.0") ("rx" "0.2.0"))
  context...:
   /home/pkgserver/racket/collects/pkg/private/content.rkt:14:0: extract-pkg-dependencies
   /home/pkgserver/racket/collects/racket/contract/private/arrow-val-first.rkt:397:3
   /home/pkgserver/pkg-index/official/update.rkt:125:4
   /home/pkgserver/racket/collects/racket/contract/private/arrow-higher-order.rkt:344:33
   /home/pkgserver/racket/collects/pkg/private/content.rkt:39:0: get-pkg-content20
   /home/pkgserver/racket/collects/racket/contract/private/arrow-val-first.rkt:397:3
   /home/pkgserver/pkg-index/official/update.rkt:114:0: update-from-content
   /home/pkgserver/racket/collects/racket/private/more-scheme.rkt:261:28
   /home/pkgserver/racket/collects/racket/private/list.rkt:264:4: loop
   /home/pkgserver/pkg-index/official/update.rkt:135:0: do-update!
   /home/pkgserver/pkg-index/official/common.rkt:138:0: run!
   /home/pkgserver/pkg-index/official/update.rkt:152:22
mflatt commented 6 years ago

I think the problem is the definition of "version string". The valid-version? function from version/utils rejects "1.0.0" and "0.2.0", because it considers the trailing ".0" redundant and therefore disallowed. I don't know where that's specified, if anywhere, though.

bennn commented 6 years ago

If it's possible, I'd be happy if valid-version changed. Rejecting 6.10.1.0 was a minor problem for me earlier this month.

mflatt commented 6 years ago

I don't think valid-version? should change. It should retain its role in checking for versions in canonical form, but that property should be documented.

It's possible that the package system should be more lenient, but there's no backward-compatibility issue here as far as I understand. I think it's better all around for version numbers to be in a checked, canonical form. If we go that way, though, besides better documentation, a better error message is needed.

rfindler commented 6 years ago

I agree that documentation and error message improvements are the right call here.

samth commented 6 years ago

I guess I'm unclear on why "canonical form" in this sense is a valuable property to check for.

rfindler commented 6 years ago

So that pkgs can put version dependencies in them without having to read an individual pkg's documentation on the subject?

samth commented 6 years ago

As long as version<=? handled the trailing ".0" correctly, allowing this seems like it would work fine for the downstream users.

mflatt commented 6 years ago

Yes, version<=? could normalize, but it's easy to imagine other uses of version numbers, such as in URLs or documentation links, and we'd have to specifically remember to normalize in those cases. (More likely, we'll forget.)

My objection is the speculative objection of a grumpy old sysadmin. But I'll stand by it, anyway.

samth commented 6 years ago

My worry is that we currently don't validate these specifications basically any time except when the package server itself runs things, leading to issues like the one linked to.

rfindler commented 6 years ago

Could we add this check to the dependency checking that raco setup is currently doing?

samth commented 6 years ago

It may already happen in that operation, but I don't think people usually run raco setup by itself.

rfindler commented 6 years ago

I don't understand: if you don't do dependency checking then many other things will go wrong and the comment that "we don't validate these specifications ..." seems, well, not helpful here?

samth commented 6 years ago

If you don't do proper dependency checking then you may get warnings on the package server. If you don't get this version spec right, then you get an error from the package system that means your package can't be installed at all.

Here's a plausible scenario that I think resulted in several packages having bad version specs in various places currently.

  1. You develop a bunch of packages, so they're all installed already.
  2. You upgrade one package, giving it a new-but-invalid version such as "0.2.0", which doesn't produce any errors when running raco setup -p <pkgname>
  3. You add deps to your package that depends on the updated package, using the same invalid version spec. raco setup -p <other-pkgname> also doesn't error.
  4. You push the changes to git, and your packages are no longer installable.

At the various intermediate points here, running raco setup (with no other arguments) will error internally because of bad version specs, which hopefully provides enough information to fix, but this doesn't happen unless you run setup on everything, which from talking to people happens pretty rarely for most package developers.

rfindler commented 6 years ago

Okay, so that means you like my suggestion that we do this check during dependency checking, then? (I'm confused.)

samth commented 6 years ago

I think we need to do this check (and others like it) in other places that people are more likely to run, as well as in the dependency checking that we currently do.