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

Implement 'scope' field for executables (#708) #3461

Merged
merged 8 commits into from May 5, 2017

Conversation

Projects
None yet
5 participants
@DanielG
Copy link
Collaborator

DanielG commented May 26, 2016

I use executables installed into '$libexecdir' with great effect in ghc-mod and cabal-helper but so far I've been using an ugly hack in Setup.hs so I figured why not get this upstreamed since it's relatively straightforward.

Now in #708 the request is for a boolean field "Is-Helper-Executable" but I figured having support for specifying an arbitrary path while using the PathTemplate variables is a lot more flexible and just seems like the consistent thing to do since there aren't any other such boolean fields anywhere in the PackageDescription apart from say 'buildable'.

I've added a check to make sure packages that end up on hackage don't have absolute paths in 'install-dir' which seems like a reasonable restriction.

I haven't tested the bits in PrettyPrint yet and I'd appreciate feedback on what is a sensible thing to do there since I just copied what is being done for 'main-is' but I'm not sure if that makes any sense.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented May 27, 2016

No comment on the functional aspects of this, but it should probably be squashed since the individual commits don't really make sense as separate pieces of functionality.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented May 27, 2016

Yes of course I was just holding off on that as to not create more spam in the linked issue since GH doesn't remove old commit references when you force push over them :)

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 28, 2016

@dcoutts, what do you think?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 28, 2016

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented May 31, 2016

I very much welcome support for private $libexec executables.

@DanielG is the suggestion that authors can specify any install location for helper/private exes, not just the libexec dir? I'm not so sure about that. Generally the split between authors and builders is that authors specify what category things are and the builder gets to decide where things go. As such I'd have thought the appropriate split in this case is to declare the exe as private, and then it'll automatically be installed into the $libexec dir (which the package builder gets to choose, with the obvious default).

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Jun 6, 2016

@dcoutts You have a point there. So what would you suggest instead? I think just having a boolean flag is kind of meh, how about a new type of section "private-executable" or "internal-executable" or something like that?

Though looking at how GNU handles this (https://www.gnu.org/prep/standards/html_node/Directory-Variables.html) it seems like a package author is meant to be able to install executables into sub-directories of $libexecdir and this was pretty much my original intent. So if I enforce install-dir being either just $bindir or something relative to $libexecdir would that work for you?

  • libexecdir
    The directory for installing executable programs to be run by other programs rather than by users. This directory should normally be /usr/local/libexec, but write it as $(exec_prefix)/libexec. (If you are using Autoconf, write it as ‘@libexecdir@’.)

    The definition of ‘libexecdir’ is the same for all packages, so you should install your data in a subdirectory thereof. Most packages install their data under $(libexecdir)/package-name/, possibly within additional subdirectories thereof, such as $(libexecdir)/package-name/machine/version.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Jun 16, 2016

@dcoutts ping.

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Jul 11, 2016

@dcoutts ping again!

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Aug 2, 2016

@DanielG sorry, don't always notice github notifications (+ holidays).

Yes, I'd say either we add a new section type, or probably easier, we add a field to the existing exe type to say that it's a private exe (much like we have non-buildable sections).

As for a subdir of $libexec, I'd suggest we actually do this automatically. Then as far as I can see there would be no need to control it manually. So just as we have a $libsubdir (such that the final libdir becomes $libdir/$libsubdir and by default the $libsubdir contains the package id to avoid clashes) then we should just follow the same pattern with $libexecdir/$libexecsubdir. The existing getLibexecDir would have to return the final libexec dir, just as getLibDir does.

Do you agree, would that remove any need for the package author to control a libexec subdir? The package builder still gets to choose of course, and by default we arrange (like libdir) so that nothing can clash between packages.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 5, 2017

Ok so here's another shot at this, I added $libexecsubdir as @dcoutts suggested and settled for adding a new field scope to executable sections to determine the installation directory. I went with public/private as the possible values since this nomenclature ("Private executables") was already used in some places in user visible output and I never really liked adding a boolean field like internal: True anyways.

The default value for the new $libexecsubdir is $abi/$pkgid, I think using $libname (as $libsubdir does) would overkill here. I also don't make it depend on the compiler like $libsubdir since I'm guessing that was just done for backwards compatibility's sake.

I'm not sure the changes I made in ProjectPlanning.hs make sense since I don't know why the installdirs are being overridden in there in the first place.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 5, 2017

Also I'm not sure if ppExe (https://github.com/haskell/cabal/pull/3461/files#diff-ef1d7da890915400ce47523b1255deb1R153) is doing the right thing, I don't really understand what the second case is doing but running cabal format on some cabal files with and without the scope field seems to work.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 5, 2017

There's still one failing test from integration-tests2:

  Successful builds
    Setup script styles: unrecognized 'configure' option `--libexecsubdir='
FAIL (3.73s)
      SetupCustomExplicitDeps
        expected to find a-0.1 in the Installed state, but it is actually in the Failed state.

Not sure how to fix that, any ideas?

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Feb 5, 2017

The error message suggests that you are passing libexecsubdir to an old version of Setup, which isn't able to recognize it. So you need to correctly handle BC with old versions of Cabal library

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 5, 2017

That's what I was afraid of. There doesn't seem to be any mechanism for backwards compatibility in the Setup.hs interface, am I missing something or is this really the first backwards incompatible change there?

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Feb 5, 2017

A sensible BC fallback is to act as if this patch did not exist, right? This can probably be done by simply not passing the flag, right? Look at filterConfigureFlags in Distribution.Client.Setup for our preexisting logic for this sort of thing.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 5, 2017

Ah, that's exactly what I was looking for.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 6, 2017

Ok, that should do it. I also added a check to make sure the spec version is correct.

@DanielG DanielG changed the title Implement 'install-dir' field for executables (#708) Implement 'scope' field for executables (#708) Feb 6, 2017

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Feb 9, 2017

@23Skidoo any chance this could still go into 2.0?

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Mar 7, 2017

Bump on this.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 7, 2017

This needs to be updated before I can consider it for 2.0.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Mar 10, 2017

@23Skidoo done. This rebases cleanly onto 2.0 with git rebase --onto 2.0 origin/master exe-install-dir, I also pushed the rebased version here if that's more convenient: https://github.com/DanielG/cabal/tree/exe-install-dir-2.0

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 10, 2017

Nice. I'll take a look later today or tomorrow.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Mar 28, 2017

@23Skidoo any news?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 28, 2017

Sorry, I was ill last week. Will try to look into this tomorrow.

@DanielG

This comment has been minimized.

Copy link
Collaborator

DanielG commented Apr 30, 2017

Ping @23Skidoo

@DanielG DanielG changed the base branch from master to 2.0 Apr 30, 2017

@23Skidoo 23Skidoo added this to the 2.0 milestone May 5, 2017

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 5, 2017

Also I'm not sure if ppExe (https://github.com/haskell/cabal/pull/3461/files#diff-ef1d7da890915400ce47523b1255deb1R153) is doing the right thing, I don't really understand what the second case is doing but running cabal format on some cabal files with and without the scope field seems to work.

Second case seems to be dead code that can be only executed when simplifiedPrinting is True. Apparently the original author (@jutaro) used that for debugging.

@23Skidoo 23Skidoo merged commit c454637 into haskell:2.0 May 5, 2017

11 checks passed

ci/sake-bot/linux-7.10.3 Downstream Travis
Details
ci/sake-bot/linux-7.4.2 Downstream Travis
Details
ci/sake-bot/linux-7.6.3 Downstream Travis
Details
ci/sake-bot/linux-7.8.4 Downstream Travis
Details
ci/sake-bot/linux-8.0.2 Downstream Travis
Details
ci/sake-bot/linux-8.0.2-parsec Downstream Travis
Details
ci/sake-bot/osx-7.10.3 Downstream Travis
Details
ci/sake-bot/osx-7.8.4 Downstream Travis
Details
ci/sake-bot/osx-8.0.2 Downstream Travis
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 5, 2017

Merged into 2.0, added a changelog note and documentation.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 5, 2017

@DanielG Thanks for your contribution! Can you please also rebase this patchset against master and do a separate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment