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

Reconsider how to specify dependencies on internal libraries #4155

Closed
ezyang opened this issue Dec 5, 2016 · 30 comments
Closed

Reconsider how to specify dependencies on internal libraries #4155

ezyang opened this issue Dec 5, 2016 · 30 comments

Comments

@ezyang
Copy link
Contributor

ezyang commented Dec 5, 2016

Right now, if I define an internal library helper in the package foobar, I refer to it by adding an extra line to build-depends:

name: foobar
version: 1.0
library helper
  ...
library
  build-depends:
    base >= 4.4,
    helper

This is... kind of "wrong" from a semantic sense. There are two big reasons: (1) the primary reason that build-depends exist is to specify version bounds on external packages, but if we are referencing an internal library, you never want to actually specify a version bound: you want to refer to the library internally, and (2) we're clobbering the global namespace of packages, you can't name a helper the same name as an external package you also want to use.

We still have an opportunity to fix this, since internal libraries have not been published in a real release of Cabal. In particular, a new feature of Backpack might be a good way to fix this: the mixins field. Here are the proposed new semantics:

  1. We define a new syntax for referring to internal libraries within a package. One possibility (to get the bikeshedding going): pkg/libname refers to the libname internal library of pkg. To refer to the current package, you can use a dot, as in ./libname, or the full package name.
  2. The build-depends syntax remains UNCHANGED. Instead, we augment the mixins syntax to accept qualified library names; e.g., mixins: foobar/helper, foobar/other-helper.
  3. Here is how module visibility works. As in Backpack today, if a library is mentioned in build-depends but not in mixins, we do the historical behavior: the public library of that package is brought into scope. Otherwise, if ANY mixin refers to a package, we no longer bring that into scope; we bring precisely only the libraries identified by mixins into scope. So for example:
name: foobar
library
  ...
library helper
  ...
executable fooexe
  build-depends: foobar
  mixins: foobar/helper

this package only brings the modules of helper into scope for fooexe, not the main library foobar. (BY THE WAY, if we hate the fact that referring to a package via a mixin causes its default "import" to not be brought into scope, we could add a new variant of build-depends for packages we want to depend on, but should not have any modules brought into scope; maybe just version-depends?)
4. Optionally, we can make it optional to specify the name of the current package in build-depends if you want to use libraries from it in mixins. So continuing the example from above,

name: foobar
library
  ...
executable fooexe
  mixins: foobar/helper

is OK (you can omit the build-depends.) This is a bit clearer. If we ever end up supporting multiple public libraries per package, you'll have to specify that package in build-depends (since we need to know to depsolve for it, and what your version constraints are.)

CC @Ericson2314

@ezyang ezyang added this to the 2.0 milestone Dec 5, 2016
@abooij
Copy link
Contributor

abooij commented Dec 6, 2016

What about foreign libraries? I can imagine hypothetical scenarios in which executables might want to depend on foreign libraries.

Though such code is probably solving the wrong problem :)

@ezyang
Copy link
Contributor Author

ezyang commented Dec 6, 2016

That would be a different kind of dependency, so it would have its own field, I think.

@Ericson2314
Copy link
Collaborator

I mentioned it on IRC, but just to log it here for posterity, the only issue I have with this is ":" being used as an analogous separator in many other contexts.

@ezyang
Copy link
Contributor Author

ezyang commented Dec 18, 2016

Yeah we can use ":". I am just a little mournful that we can no longer use "./" as an idiom for "in this namespace."

@ezyang ezyang self-assigned this Dec 30, 2016
ezyang added a commit to ezyang/cabal that referenced this issue Dec 30, 2016
Fixes haskell#4155.

Needs a doc update.

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang
Copy link
Contributor Author

ezyang commented Dec 31, 2016

I implemented this, but actually I'm pretty unhappy with the use of the mixins keyword here: it ends up being pretty confusing if you're not actually using Backpack, and also it is just so natural to put library dependencies in build-depends, it's a bit hard to kick the habit.

So my new counter-proposal is that we should fix (2), but for (1) settle for making it "manifestly obvious" what package an attached version bound corresponds to.

  1. Extend build-depends syntax to accept the form pkgname:cname bounds and :cname bounds. If you are in a package named pkg, to refer to the internal library helper, you write pkg:helper or just :helper. Because internal packages must always be qualified, there is no longer any ambiguity problem. Furthermore, if there is an attached version bound, it applies to the package that the component is contained in. This is easy for the dependency solver to read off, because the package that the bound should be applied to is directly written in the syntax. As before, package bounds on internal libraries are nonsensical and will result in check warnings, but if we ever support dependencies on internal libraries from external packages, the bounds are useful.

  2. Extend mixins syntax to accept the form pkgname:cname renamings and :cname renamings. If you specify a mixin, this overrides the default from build-depends (as before.) However, you can now specify internal libraries in mixins without declaring them in build-depends.

@hvr, @Ericson2314, @abooij, @dcoutts, @23Skidoo what do you think about this plan?

@Ericson2314
Copy link
Collaborator

Ericson2314 commented Dec 31, 2016

That sounds great! A slight nit is : as the first char gives me absolute-path vibes, so something analogous to . for the current package might still be nice---but maybe I'm biased from prior discussion and this won't confuse most users.

@hvr
Copy link
Member

hvr commented Dec 31, 2016

Having some special symbolic identfier to refer to the current package would be nice; however, if we can't agree on one (I'm not sure about the leading :int-lib-name thing either; would a lonely : denote the default library of the current package then?), it wouldn't be the worst thing to just always require the explicit pkgname:-qualifier...

@ezyang That being said, I like the general direction of (1) in #4155 (comment) ; haven't thought about the (2) part yet

@ezyang
Copy link
Contributor Author

ezyang commented Dec 31, 2016

I picked leading colon because it's how Bazel seems to do it. Bare colon seems like too much: you can just specify the name of the package by itself (which is how existing code does.) But I won't add this feature in for now. Simpler code!

Don't worry about the (2) part, it's just the "obvious" thing.

@Ericson2314
Copy link
Collaborator

Ericson2314 commented Dec 31, 2016

Ah, so all named/internal library deps will need an explicit package name, whether that library is part of the current packatge or not? That strikes me as the perfect first step :).

@ezyang
Copy link
Contributor Author

ezyang commented Dec 31, 2016

Yep.

@23Skidoo
Copy link
Member

23Skidoo commented Jan 3, 2017

@hvr, @Ericson2314, @abooij, @dcoutts, @23Skidoo what do you think about this plan?

+1 from me.

As before, package bounds on internal libraries are nonsensical and will result in check warnings, but if we ever support dependencies on internal libraries from external packages, the bounds are useful.

Do I understand correctly that pkgname:cname dependencies where pkgname != current_pkgname will result in an error as well?

@hvr
Copy link
Member

hvr commented Jan 3, 2017

Do I understand correctly that pkgname:cname dependencies where pkgname != current_pkgname will result in an error as well?

@23Skidoo at least as long as we don't support exposed convenience libraries (which would allow package families such as amazonka-* to collapse 89(!) sub-packages into a single package with multiple exposed libraries reducing the pkg-index meta-data overhead by one or two OOMs; which is also something that .deb or .rpms support; I still wonder why cabal shouldn't support this either)

@hvr
Copy link
Member

hvr commented Jan 3, 2017

I just ran the numbers; a single amazonka upload currently results in 89 auto-generated packages being added to the package index:

$ cabal list --simple-output | grep amazonka | grep -F 1.4.5
amazonka 1.4.5
amazonka-apigateway 1.4.5
amazonka-application-autoscaling 1.4.5
amazonka-appstream 1.4.5
amazonka-autoscaling 1.4.5
amazonka-budgets 1.4.5
amazonka-certificatemanager 1.4.5
amazonka-cloudformation 1.4.5
amazonka-cloudfront 1.4.5
amazonka-cloudhsm 1.4.5
amazonka-cloudsearch 1.4.5
amazonka-cloudsearch-domains 1.4.5
amazonka-cloudtrail 1.4.5
amazonka-cloudwatch 1.4.5
amazonka-cloudwatch-events 1.4.5
amazonka-cloudwatch-logs 1.4.5
amazonka-codebuild 1.4.5
amazonka-codecommit 1.4.5
amazonka-codedeploy 1.4.5
amazonka-codepipeline 1.4.5
amazonka-cognito-identity 1.4.5
amazonka-cognito-idp 1.4.5
amazonka-cognito-sync 1.4.5
amazonka-config 1.4.5
amazonka-core 1.4.5
amazonka-datapipeline 1.4.5
amazonka-devicefarm 1.4.5
amazonka-directconnect 1.4.5
amazonka-discovery 1.4.5
amazonka-dms 1.4.5
amazonka-ds 1.4.5
amazonka-dynamodb 1.4.5
amazonka-dynamodb-streams 1.4.5
amazonka-ec2 1.4.5
amazonka-ecr 1.4.5
amazonka-ecs 1.4.5
amazonka-efs 1.4.5
amazonka-elasticache 1.4.5
amazonka-elasticbeanstalk 1.4.5
amazonka-elasticsearch 1.4.5
amazonka-elastictranscoder 1.4.5
amazonka-elb 1.4.5
amazonka-elbv2 1.4.5
amazonka-emr 1.4.5
amazonka-gamelift 1.4.5
amazonka-glacier 1.4.5
amazonka-health 1.4.5
amazonka-iam 1.4.5
amazonka-importexport 1.4.5
amazonka-inspector 1.4.5
amazonka-iot 1.4.5
amazonka-iot-dataplane 1.4.5
amazonka-kinesis 1.4.5
amazonka-kinesis-analytics 1.4.5
amazonka-kinesis-firehose 1.4.5
amazonka-kms 1.4.5
amazonka-lambda 1.4.5
amazonka-lightsail 1.4.5
amazonka-marketplace-analytics 1.4.5
amazonka-marketplace-metering 1.4.5
amazonka-ml 1.4.5
amazonka-opsworks 1.4.5
amazonka-opsworks-cm 1.4.5
amazonka-pinpoint 1.4.5
amazonka-polly 1.4.5
amazonka-rds 1.4.5
amazonka-redshift 1.4.5
amazonka-rekognition 1.4.5
amazonka-route53 1.4.5
amazonka-route53-domains 1.4.5
amazonka-s3 1.4.5
amazonka-sdb 1.4.5
amazonka-servicecatalog 1.4.5
amazonka-ses 1.4.5
amazonka-shield 1.4.5
amazonka-sms 1.4.5
amazonka-snowball 1.4.5
amazonka-sns 1.4.5
amazonka-sqs 1.4.5
amazonka-ssm 1.4.5
amazonka-stepfunctions 1.4.5
amazonka-storagegateway 1.4.5
amazonka-sts 1.4.5
amazonka-support 1.4.5
amazonka-swf 1.4.5
amazonka-test 1.4.5
amazonka-waf 1.4.5
amazonka-workspaces 1.4.5
amazonka-xray 1.4.5

@23Skidoo
Copy link
Member

23Skidoo commented Jan 3, 2017

@hvr This is a convincing use case.

@ezyang
Copy link
Contributor Author

ezyang commented Jan 3, 2017

@23Skidoo Yes, as long as we don't support exposed convenience libraries.

@ezyang
Copy link
Contributor Author

ezyang commented Jan 19, 2017

Update: This patchset seems tremendously annoying to implement, so I am now inclined to can it and keep the old syntax.

@Ericson2314
Copy link
Collaborator

Ericson2314 commented Jan 19, 2017

Oh really? #4155 (comment) seems like surface-level parser tweaks? I really liked it.

@ezyang
Copy link
Contributor Author

ezyang commented Jan 19, 2017

The problem was that it was really tempting to refactor the internal representations so that #4206 would make sense (basically, Setup scripts would know how to build against internal libraries from other packages, even if no other tooling knew), but that ended up being a pretty big refactor which I got bogged down in.

@Ericson2314
Copy link
Collaborator

Gotcha. Well hopefully we can at least do the surface level parsing change before 2.0? I'd really prefer pkg:lib to be the only way to access a non-primary library, even if it is internal.

@ezyang
Copy link
Contributor Author

ezyang commented Jan 22, 2017

It's not just a surface level parsing change, as if we introduce pkg:lib as a thing, it will now be possible to name an internal library the same as an external package, and use both simultaneously. So there's going to be some substantive changes.

@Ericson2314
Copy link
Collaborator

Ericson2314 commented Jan 22, 2017

Hmm, one could just add an assertion that those namespaces be disjoint? Or at least that no referenced external library has the same name even if there is another out there somewhere in the project plan?

@Ericson2314
Copy link
Collaborator

@ezyang have you pushed a branch with your work there? I'd be happy to dumb it down for a 2.0 stop-gap :)

ezyang added a commit to ezyang/cabal that referenced this issue Jan 25, 2017
Fixes haskell#4155.

Needs a doc update.

Signed-off-by: Edward Z. Yang <[email protected]>
@Ericson2314
Copy link
Collaborator

Ericson2314 commented Feb 17, 2017

Won't the current syntax be grandfathered in then? I'll have some time early next week. Otherwise I'd truely appreciate if anyone else could take a stab at it.

Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Feb 22, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Feb 22, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Feb 24, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Mar 1, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Mar 3, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Mar 15, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Mar 18, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Mar 18, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Apr 5, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Apr 27, 2017
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
@23Skidoo 23Skidoo modified the milestones: 2.2, 2.0 May 5, 2017
@23Skidoo
Copy link
Member

23Skidoo commented May 5, 2017

Remilestoned.

kmicklas pushed a commit to kmicklas/cabal that referenced this issue Jan 26, 2018
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
kmicklas pushed a commit to kmicklas/cabal that referenced this issue Jan 26, 2018
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
Ericson2314 pushed a commit to Ericson2314/cabal that referenced this issue Mar 7, 2018
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
fgaz pushed a commit to fgaz/cabal that referenced this issue Jul 20, 2018
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
@23Skidoo 23Skidoo modified the milestones: 2.2, 2.4 Aug 29, 2018
@23Skidoo 23Skidoo modified the milestones: 2.4, 2.4.1 Sep 17, 2018
@23Skidoo 23Skidoo modified the milestones: 2.4.1.0, 2.4.2.0 Apr 26, 2019
@phadej phadej modified the milestones: 2.4.2.0, 3.4 Nov 27, 2019
@fgaz
Copy link
Member

fgaz commented Jun 29, 2020

@phadej isn't this fixed for .cabal 3.4 with one of your recent patches?

@phadej
Copy link
Collaborator

phadej commented Jun 29, 2020

@fgaz yes good find. This is fixed by fix for #6083 (#6907).

@phadej phadej closed this as completed Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants