Skip to content

setup sdist places implementation-dependent preprocessor output in the source bundle #130

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

Closed
bos opened this issue May 24, 2012 · 21 comments

Comments

@bos
Copy link
Contributor

bos commented May 24, 2012

(Imported from Trac #137, reported by ross on 2007-06-02)

e.g. happy -agc output in the haskell-src package.

Suggested new scheme: http://www.haskell.org/pipermail/cabal-devel/2007-May/000529.html.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by ross on 2007-06-02)

see also #92: issues with 'clean'.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @igfoo on 2007-06-07)

It also includes files generated by "Setup configure", e.g. HsGLUT.h in the GLUT package.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @dcoutts on 2008-06-19)

Replying to @igfoo:

It also includes files generated by "Setup configure", e.g. HsGLUT.h in the GLUT package.

Which is because Cabal has no idea that HsGLUT.h is the result of HsGLUT.h.in.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by ross on 2008-06-19)

Replying to @dcoutts:

Replying to @igfoo:
It also includes files generated by "Setup configure", e.g. HsGLUT.h in the GLUT package.

Which is because Cabal has no idea that HsGLUT.h is the result of HsGLUT.h.in.

Maybe #152 wasn't a bug.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @dcoutts on 2008-06-19)

Or maybe we should assume that foo is generated (by an unknown pre-processor) by foo.in (if foo.in exists) and so it is foo.in that should go in the sdist and not foo.

Though that also violates our rule about putting generated files in dist/ and not back in the source tree.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @igfoo on 2008-06-19)

Incidentally, include/HsGLUT.h is in extra-tmp-files; should Cabal look at that when deciding what to put in the sdist?

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @dankna on 2008-06-20)

Attached, please find three patches for Cabal and one for
Cabal-Install, in "darcs send" format. Also find a change to Happy,
in the form of an entire Setup.lhs file to replace the corresponding
file in Happy-1.18.4 (I guess I must not have been able to find the
original repo for Happy - I can prepare a diff if you find that
easier). These implement the stuff we talked about with regard to
preprocessor output. Hopefully you'll like the way I did it. I'm
afraid it's only easy for me to test with ghc, but it /should/ support
all compilers, since I did make appropriate changes to all of their
backends. If this is a problem I can try to get the others set up,
but I hoped you might already be set for that and able to do it more
quickly than I could.

There are three main pieces to the patch, as the commit messages will
explain. Briefly, the first piece simply redirects the output, the
second piece makes sure the output is included in the sdist tarball,
and the third piece adds a user hook to implement the
subdirectory-naming policy we discussed.

With the redirection of the output, I found that by doing it at the
proper layer it wasn't necessary to add any new code to compare
timestamps; the existing codepath appeared to handle it. This could
use more testing; I used Happy as my test case.

With the inclusion in sdist, I added a new parameter to prepareTree,
which necessitated the change to Cabal-Install. As I explained in a
comment somewhere, it includes ALL immediate subdirectories of
preprocessed/, but within those, ONLY the files that have names which
could have been generated by a known preprocessor from one of the
included source files.

With the subdirectory-naming, I added a new user hook that implements
the concept of "specializing" the preprocessed output on a set of
variables. Rather than defining a new vocabulary of variables which
would then become maintenance overhead, I reused PathTemplateVariable?
for this purpose. It constructs a subdirectory name as a canonical
representation of the set of variables specialized on, with their
values. It uses URL-escaping for characters that we can't use for
various reasons.

I also made a trivial change to Happy's Setup.lhs, to test the
specialization feature. I've included that one too; once the Cabal
changes are released, either you or I can submit it to the Happy
maintainer for inclusion there. It doesn't actually implement using
Happy's --ghc option; it merely specializes the build properly,
outputting into "compiler=ghc-6.blahblahblah" if the compiler is GHC
or into "generic" otherwise. It does this by installing a
preprocesserSpecialization hook which looks at the LocalBuildInfo? it's
given, checks the compiler flavor therein, and returns [CompilerVar?]
or [] as appropriate. Hopefully this is a sufficiently general
capability for the use-cases that exist in the wild.

As always, see the source for more details. I tried to follow your
coding style as I understood it; in particular when I needed to add
imports I enumerated each symbol used. I also tried to comment
everything of any importance. I'll be interested in your feedback on
how well I managed.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @dankna on 2010-04-21)

Patch for Cabal in "darcs send" format.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @dankna on 2010-04-21)

Patch for Cabal-Install in "darcs send" format.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @dankna on 2010-04-21)

For testing purposes, modified Setup.lhs for Happy 1.18.4.

@bos
Copy link
Contributor Author

bos commented May 24, 2012

(Imported comment by @nomeata on 2010-04-21)

Is there still something happening on this front? It seems that, for example, uuagc could greatly benefit if Setup sdist would include all the generated .hs files in the tarball required to bootstrap the program; the current scheme (a separate uuagc-bootstrap package) feels like unnecessary overkill to me.

@UnkindPartition
Copy link
Contributor

I noticed that this has been changed: https://hackage.haskell.org/package/haskell-src-exts-1.16.0/haskell-src-exts-1.16.0.tar.gz doesn't contain dist/. Is that right? When (at what cabal version) was it changed, exactly?

@23Skidoo
Copy link
Member

@feuerbach You probably didn't have happy in PATH when you ran sdist.

@23Skidoo
Copy link
Member

@feuerbach I looked at the code a bit more and it looks like the following happens:

  • cabal sdist never runs happy since it's marked as non-portable preprocessor.
  • However, if you run cabal build before sdist, the generated file will be included in the tarball.

@UnkindPartition
Copy link
Contributor

That's so weird. Essentially, the output of cabal sdist depends on some random circumstances (whether you've run cabal clean before cabal sdist, say).

@UnkindPartition
Copy link
Contributor

@23Skidoo but thanks for shedding some light on this :)

@23Skidoo
Copy link
Member

IIRC when we discussed this with @dcoutts the plan to fix this was to associate metadata with files produced by non-portable preprocessors. Current scheme does feel like a hack.

@IreneKnapp
Copy link

Oh, huh. I'm glad this is still under active discussion, I was never really satisfied with my now-bitrotten patches either. Good luck. :)

@23Skidoo
Copy link
Member

23Skidoo commented Nov 1, 2014

For completeness, nice article by @feuerbach that explains why current behaviour is problematic: http://ro-che.info/articles/2014-03-08-happy-alex-ghc-7.8

@ezyang
Copy link
Contributor

ezyang commented Sep 8, 2016

I milestoned this | because, although this is a clear bug, people mostly seem to deal.

@23Skidoo 23Skidoo removed their assignment Sep 13, 2016
@typedrat
Copy link
Collaborator

#5389 ended this going forward.

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