Skip to content

Store hie files #9019

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

Merged
merged 17 commits into from
Jun 25, 2023
Merged

Store hie files #9019

merged 17 commits into from
Jun 25, 2023

Conversation

nlander
Copy link
Contributor

@nlander nlander commented Jun 12, 2023


Please include the following checklist in your PR:

Bonus points for added automated tests!

@nlander
Copy link
Contributor Author

nlander commented Jun 12, 2023

This should solve #8685

@nlander
Copy link
Contributor Author

nlander commented Jun 12, 2023

I think these changes probably should go in the changelog, but I don't know where. I don't see any release notes for the upcoming version of Cabal yet. Should I create them?

Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

Thank you @nlander, see changelog.d/pr-8662 for an example changelog.

Comment on lines 1314 to 1315
-- This is similar to 'findModuleFileEx' but doesn't fail if the file
-- corresponding to the module is missing, instead returning Nothing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you avoding changing findModuleFileEx to avoid a breaking change? Because, if we have already breaking changes, I'd prefer to change findModuleFileEx to suit the new usecase and return Maybe (FilePath, FilePath).

In case we decide to introduce two new functions, I'd like to them to share a common implementation rather than duplicating code. E.g. findModuleFileEx should call findAvailableModuleFileEx.

@angerman
Copy link
Collaborator

Shouldn't this reuse the plug-in machinery we have for modpak files? I'd be much happier to see this using the generic plugin data infrastructure than hardcoding yet another file type.

HIE files are not essential haskell files. They are Haskell Idea Engine is imo much closer to the plugin logic than for example S, h, c, hs, o, hi files. They are conceptually optional.

@angerman
Copy link
Collaborator

#8662 seems like the ideal solution. Also wrt to what @andreabedini remarked here: #8685 (comment)

@andreabedini
Copy link
Collaborator

Thanks @angerman, I agree that after #8662 we have a much more general mechanism in place. @nlander have you tried getting ghc to write hie files in extra-compilation-artifacts?

@wz1000
Copy link
Contributor

wz1000 commented Jun 13, 2023

I think extra-compilation-artifacts is not the right place for this as these files are natively generated by GHC, and so require a GHC option in order to control where they are generated, just like .o and .hi files. Unlike a plugin, we cannot easily change GHC to write them in extra-compilation-artifacts instead.

@angerman
Copy link
Collaborator

@wz1000 why can we not easily change GHC? The hie files are not required for compilation, are the now used for compilation too? Why does cabal need to know about hie files?
I do not understand why cabal itself should be concerned with hie files? They are opaque to cabal, no?

@andreabedini
Copy link
Collaborator

andreabedini commented Jun 13, 2023

Adding -hiedir=$libdir/extra-compilation-artifacts when -fwrite-ide-info is also passed seems to be the kind of things Distribution.Simple.GHC does. This is a bash POC that would do the same thing.

#!/usr/bin/env bash

args=("$@")
outputdir=""
doit=0

while [[ "$#" -gt 0 ]]; do
    case $1 in
        -outputdir) outputdir="$2"; shift ;;
        -fwrite-ide-info) doit=1; shift ;;
    esac
    shift
done

if [[ $doit -ne 0 ]]; then
  echo "Running ghc ${args[*]}" -hiedir "${outputdir:+$outputdir/}extra-compilation-artifacts"
  ghc "${args[@]}" -hiedir "${outputdir:+$outputdir/}extra-compilation-artifacts"
else
  ghc "${args[@]}"
fi

@nlander nlander requested a review from andreabedini June 14, 2023 17:54
@nlander
Copy link
Contributor Author

nlander commented Jun 14, 2023

@andreabedini @angerman I think this is ready for another look. I wonder if I could clean up the codebase by assigning the string "extra-compilation-artifacts" to a constant variable instead of repeating the same string literal throughout the codebase. Where would be a good place to put such a constant?

@andreabedini
Copy link
Collaborator

@nlander Thank you for reworking this <3
Yes, using a constant everywhere is a great idea! (perhaps with the exception of the tests, which I'd leave hardcoded). Maybe Cabal/src/Distribution/Simple/Setup/Common.hs could be an appropriate place?

@fgaz @ulysses4ever Are you ok with this design?

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

My completely uninformed opinion is that it is good. Thank you.

I wonder if and how the notion of artifacts here may be related to the one used in #8696. @bgamari had a bunch of question there, so maybe he'd be interested to take a look at this PR.

@ulysses4ever
Copy link
Collaborator

@nlander if you have capacity to look into #8696, it may bring some insights into how to make your contribution a bit more general than "just" HIE files. The thing is: various people want Cabal to produce various artifacts. It'd be good to design a mechanism that would be reusable for different purposes. I don't argue that the current attempt is not general enough, I just ask you to try to look into prior art to see if you think something could be improved in your approach.

@nlander
Copy link
Contributor Author

nlander commented Jun 16, 2023

@ulysses4ever I took a look at #8696 and I wasn't able to get many insights for this PR specifically. I think the general solution for getting cabal to produce any optional build artifacts at this point is put them in extraCompilationArtifacts. More generally though, #8967 is relevant to my Summer of Haskell project (I think), as hls will need to verify that an installed package does in fact have the .hie files generated. I will continue looking into this.

@nlander
Copy link
Contributor Author

nlander commented Jun 20, 2023

@andreabedini What still needs to happen before this can be merged?

Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

Thank you for pinging me again @nlander! It's 👍 for me.

@andreabedini andreabedini added the squash+merge me Tell Mergify Bot to squash-merge label Jun 21, 2023
@nlander nlander force-pushed the store-hie-files branch 2 times, most recently from 2aea4d9 to 63e2369 Compare June 22, 2023 03:44
@andreabedini
Copy link
Collaborator

D:\a\cabal\cabal\cabal-testsuite\PackageTests\MultiRepl\EnabledSucc\cabal.dist\work\.\dist\multi-out-7084\paths\pkg-a-0-inplace: removeDirectoryRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:DeleteFile "\\\\?\\D:\\a\\cabal\\cabal\\cabal-testsuite\\PackageTests\\MultiRepl\\EnabledSucc\\cabal.dist\\work\\dist\\multi-out-7084\\paths\\pkg-a-0-inplace": permission denied (The process cannot access the file because it is being used by another process.)

This doesn't seem related to the CI. Maybe a few spins will fix it 😂. Give a shout if you need help @nlander.

@nlander
Copy link
Contributor Author

nlander commented Jun 22, 2023

@andreabedini I have been able to reproduce this test failure on my Windows machine on master. How should I proceed?

@ulysses4ever
Copy link
Collaborator

Please, rebase. We just merged supposed fix to master.

@nlander nlander force-pushed the store-hie-files branch 2 times, most recently from 7717a2f to d180c20 Compare June 22, 2023 18:00
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jun 25, 2023
@mergify mergify bot merged commit 6df9ab4 into haskell:master Jun 25, 2023
@@ -576,6 +578,7 @@ componentGhcOptions verbosity implInfo lbi bi clbi odir =
, ghcOptFfiIncludes = toNubListR $ includes bi
, ghcOptObjDir = toFlag odir
, ghcOptHiDir = toFlag odir
, ghcOptHieDir = bool NoFlag (toFlag $ odir </> extraCompilationArtifacts) $ flagHie implInfo
Copy link
Contributor

@bgamari bgamari Sep 9, 2023

Choose a reason for hiding this comment

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

It seems a bit unfortunate to be placing files directly in extra-compilation-artifacts. My hope when we agreed on this design was that users would instead place things in distinct subdirectories to avoid clutter and the potential for name conflicts. I wonder if it wouldn't be better to rather place these in odir </> extraCompilationArtifacts </> "hie" or similar.

Perhaps it's not too late to revisit this given that we haven't yet released?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems reasonable to me. I will get a PR going for that, probably on Monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed by #9244

@bgamari
Copy link
Contributor

bgamari commented Sep 9, 2023

Apologies for not seeing your ping, @ulysses4ever . Indeed I have a question above.

@ulysses4ever
Copy link
Collaborator

@bgamari no worries at all and thanks a lot for attending to it!

@mpickering
Copy link
Collaborator

Would it not be desirable to add a specific option to a cabal.project file similar to documentation or debug-info in order to determine whether hie files are generated?

@nlander
Copy link
Contributor Author

nlander commented Sep 18, 2023

@mpickering If we added such an option for cabal.project, how would that combine with the -fwrite-ide-info flag? Would the user need to specify both the option and the ghc flag, or would either one by themselves result in generated hie files?

@bgamari
Copy link
Contributor

bgamari commented Sep 18, 2023

@nlander the user would just need to set ide-info: True in their cabal.project file.

@andreabedini
Copy link
Collaborator

@mpickering

Would it not be desirable to add a specific option to a cabal.project file similar to documentation or debug-info in order to determine whether hie files are generated?

IIRC that was the initial design of @nlander's PR; then we decided to reuse the existing infrastructure to store extra compilation artefacts. This in way the hie-files are installed along with the package in a predictable location.

I agree that using a subdirectory would a been a better idea but luckily @nlander was able to quickly address that.

W.r.t to ide-info: True, IMHO it is a design flaw having to add a cabal interface to every GHC feature.

My 2c.

@angerman
Copy link
Collaborator

angerman commented Sep 19, 2023

I agree with @andreabedini here. We should not need a cabal interface for each GHC feature. Where does this end?

Edit: (removed)

@mpickering
Copy link
Collaborator

@andreabedini I agree with the current design of the patch about where the hie files end up, it's the means that this gets enabled which I think could be improved.

I disagree with @andreabedini and @angerman, essentially all of GHC features are guarded by cabal features.. It's more discoverable and nicer to use. Some more options which are guarded by cabal features.. profiling, profiling-detail etc.. so it's also a matter of consistency to expose "blessed" and regularly used options in this manner.

A question, how do you propose to document to users how to generate ide-info? Where in the user guide does that go?

@andreabedini
Copy link
Collaborator

I disagree with @andreabedini and @angerman, essentially all of GHC features are guarded by cabal features.. It's more discoverable and nicer to use. Some more options which are guarded by cabal features.. profiling, profiling-detail etc.. so it's also a matter of consistency to expose "blessed" and regularly used options in this manner.

I think I understand your point and I acknowledge that things have evolved as you describe. I still feel I don't like this design though, I will have to think more about this.

A question, how do you propose to document to users how to generate ide-info? Where in the user guide does that go?

IMHO the problem is "which user guide"? How does GHC documents the feature? Hypotetically speaking, couldn't cabal defer to GHC user guide for GHC features? In my mind the value that cabal brings is in the definition of package, lib/exe/test/bench/flib components, flags, dependency resolution, build orchestration, etc. These are all GHC-independent features and still plenty! My 2c.

@sol
Copy link
Member

sol commented Sep 4, 2024

This is not documented and for me as a user it is not discoverable how to profit from this.

It's not even in the ticket description what exactly this is doing.

From a user's perspective I would expect a Cabal config field to enable HIE files (readide-info: True) and corresponding documentation that clarifies where exactly HIE files will end up.

(as has been suggested by @bgamari and @mpickering)

In its current form it's not entirely clear to me how we as a community can benefit from HIE files if:

  1. Users don't have a clear and documented way on how to generate them.
  2. Tool authors don't have an easy and documented way on how to discover them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants