tfausak / cabal-gild

:crown: Format Haskell package descriptions.
https://hackage.haskell.org/package/cabal-gild
MIT License
47 stars 5 forks source link

`cabal-gild-1.3.2` breaks `discover src` #89

Closed michaelpj closed 3 months ago

michaelpj commented 3 months ago

Not sure what's going on, but an index-state bump allowed HLS to pull in newer cabal-gild, which broke our tests. I manually tracked it down to cabal-gild-1.3.2, so I guess https://github.com/tfausak/cabal-gild/pull/84.

Here's what we have in the failing test, I don't know why this would have started failing now? https://github.com/haskell/haskell-language-server/blob/master/plugins/hls-cabal-gild-plugin/test/testdata/commented_testdata.cabal#L8

michaelpj commented 3 months ago

I can't make it work at all with newer cabal-gild.

michaelpj commented 3 months ago

Perhaps something changed to be sensitive to the working directory?

tfausak commented 3 months ago

Hmm, it works as expected for me with both 1.3.2.0 and 1.4.0.0. I did change how discovery works behind the scenes, but if you're not passing --include or --exclude then it should work the same as before. How exactly is cabal-gild being invoked?

Here's an example showing it working:

# cat example.cabal
-- cabal-gild: discover src
exposed-modules: whatever

# find . -type f
./example.cabal
./src/M.hs
./src/N.hs

# cabal-gild --version
1.4.0.0

# cabal-gild -i example.cabal
-- cabal-gild: discover src
exposed-modules:
  M
  N

It also works when called from another directory:

# cabal-gild -i gh-89/example.cabal
-- cabal-gild: discover src
exposed-modules:
  M
  N

And when using --stdin:

# cabal-gild --stdin example.cabal < example.cabal
-- cabal-gild: discover src
exposed-modules:
  M
  N

And with --stdin from another directory:

# cabal-gild --stdin gh-89/example.cabal < gh-89/example.cabal
-- cabal-gild: discover src
exposed-modules:
  M
  N
tfausak commented 3 months ago

It looks like your test failed on Ubuntu but succeeded on Windows: https://github.com/haskell/haskell-language-server/actions/runs/9594229151/job/26456359466#step:34:42

I'm not sure what to make of that. Maybe some directory or file casing is weird somewhere?

tfausak commented 3 months ago

@michaelpj can you provide more detail here? I wasn't able to reproduce this problem.

michaelpj commented 3 months ago

Sorry , I'm on holiday at the moment, but this is on my list to get back to. Possibly @fendor is also interested?

fendor commented 3 months ago

I can reproduce on linux:

> cabal install -z cabal-gild --constraints="cabal-gild ==1.3.2.0"
> cd <hls>
> cd plugins/hls-cabal-gild-plugin/test/testdata/
> cabal-gild --stdin=/home/hugin/Documents/haskell/hls/plugins/hls-cabal-gild-plugin/test/testdata/commented_testdata.cabal --input=- < commented_testdata.cabal 
cabal-version: 2.4
name: testdata
version: 0.1.0.0
author: Banana
extra-source-files: CHANGELOG.md

library
  -- cabal-gild: discover src
  exposed-modules:
  build-depends: base ^>=4.14.1.0
  hs-source-dirs: src
  default-language: Haskell2010

Does this help?

The absolute path to --stdin seems to be a problem, if I just pass in commented_testdata.cabal, it works fine!

tfausak commented 3 months ago

Ah, that's the ticket! Thank you!

If you pass an absolute path to either --input or --stdin, then Gild won't discover modules.

# find . -type f
./example.cabal
./M.hs

# cat example.cabal 
-- cabal-gild: discover .
exposed-modules: whatever

# cabal-gild --version
1.3.2.0

# cabal-gild --input example.cabal 
-- cabal-gild: discover .
exposed-modules: M

# cabal-gild --input $PWD/example.cabal
-- cabal-gild: discover .
exposed-modules:
tfausak commented 3 months ago

Ah, I think I've found the problem:

https://github.com/tfausak/cabal-gild/blob/c507ab246d93c9be71965d2073162c9466cbcfe9/source/library/CabalGild/Unstable/Action/EvaluatePragmas.hs#L93

Gild always searches from the current directory. The inclusions are patterns of files to discover. By default it's basically $ROOT/**. When using relative paths, $ROOT is often ., which is fine. However if you use an absolute path, then $ROOT would be that path, like /foo. Even if you're in /foo, the inclusion will fail because it's essentially just a string match — it doesn't resolve paths to see if they're the same.

I think this can be fixed by using makeRelative in a few key places.

fendor commented 3 months ago

I would also be trivial to pass in the relative location in HLS if you think this is a user error on the HLS side.

michaelpj commented 3 months ago

I think it's reasonable to expect that tools like this should work the same with relative and absolute paths, even if humans are more likely to use relative ones.

tfausak commented 3 months ago

I agree. It's a bug that Gild's discovery doesn't work with absolute paths.

tfausak commented 3 months ago

Should be fixed in 1.4.0.1! Let me know if you want a backport to 1.3.x.