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

cabal init --lib puts build-tools(happy, alex) generated modules twice into exposed-modules #7251

Closed
andreasabel opened this issue Jan 19, 2021 · 10 comments · Fixed by #7344
Closed

Comments

@andreasabel
Copy link
Member

andreasabel commented Jan 19, 2021

cabal init nicely auto-detects build-tools like happy and alex. However, it puts their generated modules twice into exposed-modules, like:

library
  exposed-modules: Main, Foo.Print, Foo.Lex, Foo.Skel, Foo.Par, Foo.Lex, Foo.Test, Foo.Abs, Foo.ErrM, Foo.Par

Note that Foo.Lex and Foo.Par are listed twice each.

cabal build then fails with a confusing error message:

Error:
    Module name  Foo.Lex  is exported multiple times.
    In the stanza 'library'
    In the inplace package 'DuplicateModules-0.1.0.0'

That's hard to understand. Took me a while, esp. since I didn't know where to look for the "inplace package".

I suppose a simple nub would solve this issue. Anyway, the order in exposed-modules looks random, wouldn't it anyway be nicer in alphabetical order? Then Set.toList . Set.fromList would do the job.

Tested cabal versions: 3.2.0.0 and 3.5.0.0.

Steps to reproduce (using BNFC to generate .x and .y files cheaply):

cabal install BNFC
echo "Foo. Bar ::= Integer" > Foo.cf
bnfc -dm Foo.cf
make
cabal init --lib
cabal build

Trivium: hpack had the very same problem: sol/hpack#406

@andreasabel
Copy link
Member Author

Tested cabal versions: 3.2.0.0 and 3.5.0.0.

Er, that is what I thought, but I can only reproduce it for 3.2.0.0.

For 3.5.0.0 (current master), no modules are inferred, exposed-modules only contains MyLib, which is what getModulesBuildToolsAndDeps puts there if no modules are found. Not sure why this is the case (did I not install cabal-install correctly and some errors are swallowed?).

Could someone test whether cabal-3.5.0.0 init --lib still infers a module list?

andreasabel added a commit to andreasabel/cabal that referenced this issue Jan 19, 2021
…--lib

This should fix the problem that duplicate entries end up in
exposed-modules, e.g. from Foo.x and Foo.hs (with alex as build-tool).
@fgaz
Copy link
Member

fgaz commented Jan 20, 2021

One could argue that cabal isn't totally wrong either: you do have duplicated modules in your source tree (the behavior you are observing only happens when you have both A.hs and A.x).

Silently ignoring them could lead to confusion, so I think in this case cabal should at least warn about it.
+1 for sorting.

@fgaz
Copy link
Member

fgaz commented Jan 20, 2021

For 3.5.0.0 (current master), no modules are inferred, exposed-modules only contains MyLib, which is what getModulesBuildToolsAndDeps puts there if no modules are found. Not sure why this is the case (did I not install cabal-install correctly and some errors are swallowed?).

Cannot reproduce, but note that the default source directory was moved from . to src, so that may be why your modules weren't detected.

@andreasabel
Copy link
Member Author

@fgaz:

One could argue that cabal isn't totally wrong either: you do have duplicated modules in your source tree (the behavior you are observing only happens when you have both A.hs and A.x).

Silently ignoring them could lead to confusion, so I think in this case cabal should at least warn about it.

Not sure I follow here. After stripping the extension it is simply a duplicate entry in a list, this can never be interpreted as two different modules.

I think it is safe to assume that if you have both A.hs and A.x, then the former is generated from the latter via Alex.
If you would have A.hs and A.lhs, then this would have been a problem already during your development, because module name A is then ambiguous.

cabal init should be useful for a scenario where you have developed something without cabal, and that might include uses of alex, happy etc., and then you cabalize it afterwards. If you have developed without cabal, then it is natural that for A.x you have generated A.hs using alex in the same directory.

@andreasabel
Copy link
Member Author

@fgaz:

Cannot reproduce, but note that the default source directory was moved from . to src, so that may be why your modules weren't detected.

Thanks, that helps!

Btw, I noticed that cabal init --source-dir=. does not do anything meaningful unless I also pass --lib. Shouldn't the following be implications?

Flag Implied flag
--source-dir --lib
--application-dir --exe
--test-dir --tests

Also, shouldn't --lib --exe be the same as --libandexe (and the latter be redundant)?
(For all of these, we should maybe open new issues.)

@fgaz
Copy link
Member

fgaz commented Jan 22, 2021

Not sure I follow here. After stripping the extension it is simply a duplicate entry in a list, this can never be interpreted as two different modules.

That's the point I'm making: it kinda is a duplicate entry already

I think it is safe to assume that if you have both A.hs and A.x, then the former is generated from the latter via Alex.
If you would have A.hs and A.lhs, then this would have been a problem already during your development, because module name A is then ambiguous.
cabal init should be useful for a scenario where you have developed something without cabal, and that might include uses of alex, happy etc., and then you cabalize it afterwards. If you have developed without cabal, then it is natural that for A.x you have generated A.hs using alex in the same directory.

In all those cases a warning seems sensible to me.
Specifically, in the case when the .hs is generated from alex/..., the generated file shouldn't be included in the package, and the user should remove it when cabalizing the package.

@andreasabel
Copy link
Member Author

Specifically, in the case when the .hs is generated from alex/..., the generated file shouldn't be included in the package, and the user should remove it when cabalizing the package.

This does not work well, because from the generated .hs-file cabal init can infer external dependencies it cannot infer from the .x file. (I tried this.) Please experiment with the script at #7253 (comment) to convince yourself of the best strategy.

It is important to keep in mind that cabal init is a heuristics and as such it should implement the smartest default behavior with the most comfort to the user.

That's the point I'm making: it kinda is a duplicate entry already

Please sketch a realistic scenario where the user would shoot themselves in the foot with my PR.

@fgaz
Copy link
Member

fgaz commented Jan 22, 2021

This does not work well, because from the generated .hs-file cabal init can infer external dependencies it cannot infer from the .x file.

That's why it should be a warning: the dependencies are inferred and the package generated, then the user is reminded to delete the generated file

Please sketch a realistic scenario where the user would shoot themselves in the foot with my PR.

  • both M.x and M.hs are present
  • cabal init does not warn
  • the user modifies M.x, builds (which does not update the .hs), and git adds both files or does a sdist (which will include both)
  • M.hs is now out of sync, since the user is no longer invoking alex directly, but still included in repos and tarballs

edit: Of course this is already possible if the user deletes the duplicate module without also remembering to delete the generated file

@andreasabel
Copy link
Member Author

andreasabel commented Jan 26, 2021

Have you tried this in practice?

What I observe:

  • cabal sdist indeed packs both M.x and M.hs. So M.hs can indeed be outdated.
  • To test, I put some garbage into M.hs
  • Then I ran cabal sdist, unpacked the archive somewhere and ran cabal build
  • cabal build ran fine, obviously ignoring M.hs and rebuilding it from M.x

This confirms my point of view.

I am happy to be disproven, but please test your counter scenario first.

UPDATE: I think a module M that can resolve to several source files, like M.x and M.hs, is not a problem of cabal init but has to be handled in upstream (in cabal build). And indeed, it is handled properly in upstream. So cabal init does not have to worry about that (hypothetical) problem.

@fgaz
Copy link
Member

fgaz commented Jan 26, 2021

I did test my scenario, but perhaps I wasn't clear enough. We are describing the same thing: the .hs stays there, out of sync, but does not cause build failures, it's just garbage data.

I think we are in disagreement only on whether the fact that there is an unused and outdated .hs in the sdist/vcs is a problem or not

edit: and on whether init is a good place to warn about it. I'd say both init and sdist should, since adding the warning in init is easy enough (the data is all there)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants