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

Clarify build-tool-depends a bit more #5561

Merged
merged 4 commits into from
Sep 29, 2018

Conversation

domenkozar
Copy link
Collaborator


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

@@ -2063,16 +2063,22 @@ system-dependent values for these fields.
.. pkg-field:: build-tool-depends: package:executable list
:since: 2.0

A list of Haskell programs needed to build this component.
A list of Haskell executabes needed to build this component.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo at "executabes"


Cabal can make sure that specified programs are built and on the ``PATH`` before building the component in question.
It will always do so for internal dependencies, and also do so for external dependencies when using Nix-style local builds.
Cabal makes sure that specified programs are built and provided on the ``PATH`` before building the component in question under following conditions:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could while at it also address #5418 (comment)


a) For Nix-style local builds, both internal and external dependencies.
b) For old-style builds, only for internal dependencies [#old-style-build-tool-depends]_.
It's up to the user to specify needed executables in this case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not "specify" but rather "provide" (in the sense of installing them and making them available via $PATH)

@domenkozar
Copy link
Collaborator Author

@hvr I hope I have addressed all of your comments.

@domenkozar domenkozar force-pushed the docs-build-tool-depends branch from 9f1bde6 to ca91884 Compare September 3, 2018 18:43
Copy link

@gilligan gilligan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I guess all relevant information is per se available there is not a single example.

Any objections to including at least one example with, and one example without version bounds?

Edit: of course only of valid build-tool-depends strings and nothing more.

@domenkozar
Copy link
Collaborator Author

Examples! Yes, I'll add those.

@hvr
Copy link
Member

hvr commented Sep 4, 2018

of course only of valid build-tool-depends strings and nothing more.

well, I think it'd be more illustrative to show a minimal example which involves an executable and a test-suite, where the test-suite uses build-tool-depends to get access to the intra-package executable (without version bounds obviously), and you can combine that with having the executable refer to some external build-tools such as markdown-unlit (with version bounds)

Copy link
Collaborator

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I like this better. Especially that new bit at the end. Thanks Domen!

@@ -2063,21 +2063,31 @@ system-dependent values for these fields.
.. pkg-field:: build-tool-depends: package:executable list
:since: 2.0

A list of Haskell programs needed to build this component.
A list of Haskell executables needed to build this component. Executables are provided during the whole duration of the build,
so this field can be used for executables needed during :pkg-section:`test-suite` as well.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err what do you mean? The tests can invoke the executables when run? I suppose they do work for that, though I hope to make a different sort of dependency for that httpsaskell/cabal/issues/5411 . Either way, I hope the vast majority of use-cases like hspec-discover really are just used when building the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :) Anything to clarify here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ericson2314 btw, assuming we get run-tool-depends at some point; we'd still need to emulate the current build-tools-depends which conflatedly implies the future run-tool-depends w/ cabal-version:2.* packages; so at that point, @domenkozar wording will still be valid for packages using the respective cabal-version:-values which still had these semantics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, that sentence implies that running the test-suite is considered part of the build-phase; I think it's worth spelling this out as otherwise it might not be obvious that the respective $PATH-augmentation will also be in effect during the invocation of the test-suite (via e.g. cabal new-test)

Copy link
Contributor

@quasicomputational quasicomputational Sep 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. So cabal new-run test:blah and cabal new-test test:blah will behave differently wrt the availability of things listed in build-tool-depends?

Edit: Experimentally verified that yes, they do behave differently. That's worrying if we're recommending new-run as the way to pass arguments to tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quasicomputational you picked up on my subtle hint... ;-)

I consider this a bit of a bug, and I seem to recall we already talked about this in another issue; i.e. ideally

  • cabal new-run test:x behaves close to cabal new-test x
  • cabal new-run bench:x behaves close to cabal new-bench x

as I'd consider it merely a different interface to invoking the test-suites; but the $PATH setup ought to be compatible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right insofar that there's a bug, I consider the new-run behavior correct :D.

External dependencies can (and should) contain a version bound like conventional :pkg-field:`build-depends` dependencies.
Internal deps should not contain a version bound, as they will be always resolved within the same configuration of the package in the build plan.
Specifically, version bounds that include the package's version will be warned for being extraneous, and version bounds that exclude the package's version will raise an error for being impossible to follow.
All executables defined in the given Cabal file are termed as *internal* dependency as opposed to the rest which are *external* dependency.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependencies


.. [#old-style-build-tool-depends] Some packages (ab)use :pkg-field:`build-depends` on old-style builds, but this has a few major drawbacks:

- using Nix-style builds it's considered an error if you depend on a exe-only package via build-depends: the solver will refuse it
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is full sentence, so I'd punctate it as such like the others


- it may or may not place the executable on $PATH

- it does not ensure correct version of the package is installed, you might end up overwriting versions with each other
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the correct version

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, so you might

2. Internal depenedencies should not contain a version bound, as they will be always resolved within the same configuration of the package in the build plan.
Specifically, version bounds that include the package's version will be warned for being extraneous, and version bounds that exclude the package's version will raise an error for being impossible to follow.

Cabal makes sure that specified programs are built and provided on the ``PATH`` before building the component in question under following conditions:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cabal try to makes sure that all specified programs are built and provided on the PATH before building the component in question, but can only do so for Nix-style builds. Specifically:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found something like that easier to read


Cabal can make sure that specified programs are built and on the ``PATH`` before building the component in question.
It will always do so for internal dependencies, and also do so for external dependencies when using Nix-style local builds.
Cabal tries to make sure that all specified programs are built and provided on the ``PATH`` before building the component in question, but can only do so for Nix-style builds. Specifically:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing worth mentioning explicitly is that those tools' are prepended to the search $PATH; that's important as otherwise a globally same-called executable could interfere (concrete example: if you happen to have a system-wide installed alex executable version 3.1.3 in /usr/bin, but your build-tools-depends: alex:alex ^>= 3.2.4 you'd certainly want to use the one provisioned by new-build satisfying the version constraint to take precedence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"are built" ... does this mean cabal will do the building automatically or that the user is expected to do the building independently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fommil would "are atomically built" clarify the intent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is important addition. I would like to see in the documentation how to specify constraints for build-tool-depends.

@domenkozar
Copy link
Collaborator Author

Hey all, I haven't given up on this, thanks for all the feedback. Going to revisit this today.

@domenkozar
Copy link
Collaborator Author

@hvr @Ericson2314 @gilligan @fommil @chshersh addressed comments, I'm not entirely sure about the examples, they should probably be full-blown otherwise they provide little insight.

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-7.10.3"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-8.0.2"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-7.6.3"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-7.8.4"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-8.2.2"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-8.4.3"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"linux-8.4.3-fdebug-expensive-assertions"
}
haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 28, 2018
"url":"pull/5561",
"account":"haskell",
"repo":"cabal",
"commit": "96d59bed00e679bf99fdc9251dc6c65c947068c3",
"tag":"osx-7.8.4"
}
@hvr hvr merged commit c070d6a into haskell:master Sep 29, 2018
@hvr
Copy link
Member

hvr commented Sep 29, 2018

@domenkozar Thank you! I've merged the current state so we can get an incremental improvement in earlier rather than later (so we can start pointing to the official docs); for the examples let's do a new PR if you're up to it! :-)

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.

7 participants