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

Remove check for the dependencies tags to be in package-sets, but let Bower complain about it #303

Closed
wants to merge 6 commits into from

Conversation

f-f
Copy link
Member

@f-f f-f commented Jul 8, 2019

As we figured yesterday in #289 (comment), bower install would fail anyways if this check would fail, but we remove the limitation that all dependencies of a package must be in the official package set.

cc @hdgarrood @Dretch

@f-f f-f requested a review from Dretch July 8, 2019 16:36
@Dretch
Copy link
Contributor

Dretch commented Jul 8, 2019

Just a couple of thoughts:

  • bower install succeeding doesn't actually mean that sensible stuff was installed. The purescript-bob repo in Bower might not be the same repo as pointed to by bob in the (with additions) package set. This might not really matter in practice, though.
  • It might be useful to generate bower.json dependencies for branch/hash dependencies like:
let additions =
      { spec-reporter-xunit =
          mkPackage
          [ "spec", "node-fs", "transformers" ]
          "https://github.com/purescript-spec/purescript-spec-reporter-xunit.git"
          "520025e148b1ea83ac0413f514511c796d3df64d"
      }

Right now this generates the broken dependency entry:

"purescript-spec-reporter-xunit": "^520025e148b1ea83ac0413f514511c796d3df64d"

but it could generate the valid entry:

"purescript-spec-reporter-xunit": "https://github.com/purescript-spec/purescript-spec-reporter-xunit.git#520025e148b1ea83ac0413f514511c796d3df64d"

This might not be useful for libraries (which perhaps should not normally depend on out-of-package set versions?) but supporting this means that bump-version can also be used for applications (maybe useful).

We could check each package in the package set to see if the package in Bower with the same name is pointing to the same Git repo (via bower lookup purescript-xxx), and if not then generate the URL#version syntax instead of the regular bower dependency shorthand. This might be overkill though.

@f-f
Copy link
Member Author

f-f commented Jul 9, 2019

@Dretch

bower install succeeding doesn't actually mean that sensible stuff was installed. The purescript-bob repo in Bower might not be the same repo as pointed to by bob in the (with additions) package set. This might not really matter in practice, though.

Yeah, it looks like we could prevent this (obviously broken) situation by doing a bower lookup for every package as you suggested, and trying to match the URL (the users might use git://.. URLs in their package set though, which will not satisfy an exact match, but I'm fine with failing and telling the user to use HTTP urls in that case. Implementing some parsing logic might be a better solution, but it's more work and might still have corner cases)

On the other hand if there is such a package mismatch most likely the published package is not going to compile and the user will realise and fix it immediately..

but it could generate the valid entry:

Actually this looks like the right fix for this PR: we should do a bower info purescript-${packageName}#${packageVersion} for every dependency, and if that succeeds then we put it in the bower.json.

This allows us to also work with commit hashes (which I'm fine supporting): if the packageVersion is a tag bower info will return us the version field in the JSON result, so if it's not we know that we are not dealing with a tag so we can generate the URL syntax for the package version

Examples:

$ bower info purescript-simple-json#7.0.0

{
  name: 'purescript-simple-json',
  license: 'MIT',
  repository: {
    type: 'git',
    url: 'git://github.com/justinwoo/purescript-simple-json.git'
  },
  ignore: [
    '**/.*',
    'node_modules',
    'bower_components',
    'output'
  ],
  dependencies: {
    'purescript-prelude': '^4.1.1',
    'purescript-typelevel-prelude': '^5.0.0',
    'purescript-record': '^2.0.1',
    'purescript-variant': '^6.0.1',
    'purescript-nullable': '^4.1.1',
    'purescript-foreign-object': '^2.0.3',
    'purescript-globals': '^4.0.0',
    'purescript-foreign': '^5.0.0',
    'purescript-exceptions': '^4.0.0',
    'purescript-arrays': '^5.3.0'
  },
  devDependencies: {
    'purescript-assert': '^4.1.0',
    'purescript-generics-rep': '^6.1.1'
  },
  homepage: 'https://github.com/justinwoo/purescript-simple-json',
  version: '7.0.0'
}
$ bower info purescript-simple-json#d6c4163f6ad65be97b148ebf86c900cb821da5a8

{
  name: 'purescript-simple-json',
  license: 'MIT',
  repository: {
    type: 'git',
    url: 'git://github.com/justinwoo/purescript-simple-json.git'
  },
  ignore: [
    '**/.*',
    'node_modules',
    'bower_components',
    'output'
  ],
  dependencies: {
    'purescript-prelude': '^4.1.1',
    'purescript-typelevel-prelude': '^5.0.0',
    'purescript-record': '^2.0.1',
    'purescript-variant': '^6.0.1',
    'purescript-nullable': '^4.1.1',
    'purescript-foreign-object': '^2.0.3',
    'purescript-globals': '^4.0.0',
    'purescript-foreign': '^5.0.0',
    'purescript-exceptions': '^4.0.0',
    'purescript-arrays': '^5.3.0'
  },
  devDependencies: {
    'purescript-assert': '^4.1.0',
    'purescript-generics-rep': '^6.1.1'
  },
  homepage: 'https://github.com/justinwoo/purescript-simple-json'
}

@Dretch
Copy link
Contributor

Dretch commented Jul 13, 2019

@f-f

Actually this looks like the right fix for this PR: we should do a bower info purescript-${packageName}#${packageVersion} for every dependency, and if that succeeds then we put it in the bower.json.

This allows us to also work with commit hashes (which I'm fine supporting): if the packageVersion is a tag bower info will return us the version field in the JSON result, so if it's not we know that we are not dealing with a tag so we can generate the URL syntax for the package version

Sounds fine to me.

I can implement this if you would like?

@f-f
Copy link
Member Author

f-f commented Jul 13, 2019

@Dretch that would be great thanks!

@f-f
Copy link
Member Author

f-f commented Jul 15, 2019

@Dretch I'm going to close this PR, could you open a new one from this branch? (so that the squashed commit is rightly attributed to you)

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.

2 participants