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

Customize the unitId used for the fake internal component #1435

Merged
merged 6 commits into from Feb 27, 2021

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Feb 24, 2021

ghcide sets a custom unit id for the internal component. While this is mostly an internal implementation detail, it leaks out when trying to reuse interface files produced by ghc, which will need to be configured with --this-unit-id fake_uid. This is fine, but what if in the future ghcide changes the fake uid string?

This PR adds a flag to control the fake uid string which can be used to get some protection against future ghcide changes.

Additionally, we should probably change the default value to "main", which is the name ghc uses for the default component, in order to make the scenario above, reusing interface files produced by ghc, work out of the box in most cases. Thoughts @fendor @wz1000 ?

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM.

I think "main" fake_uid can work, at least I don't know of a blocker from the top of my head.

@pepeiborra
Copy link
Collaborator Author

LGTM.

I think "main" fake_uid can work, at least I don't know of a blocker from the top of my head.

Perfect, let's change it and see what the test suite says

@pepeiborra pepeiborra force-pushed the custom-unit-id branch 2 times, most recently from f9ca4ca to 76ea473 Compare February 24, 2021 11:11
Copy link
Collaborator

@wz1000 wz1000 left a comment

Choose a reason for hiding this comment

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

Can we set the unit id per component? hiedb expects a unique (ModuleName, UnitId) pair which causes some issues with main modules: wz1000/HieDb#13

@fendor
Copy link
Collaborator

fendor commented Feb 24, 2021

I remember the unit id was set to the same for each component for a reason. However, writing a test for it and proving that it does or doesn't work is probably the best course of action.

@pepeiborra
Copy link
Collaborator Author

I think the answer is no, see:

-- There are several places in GHC (for example the call to hptInstances in
-- tcRnImports) which assume that all modules in the HPT have the same unit
-- ID. Therefore we create a fake one and give them all the same unit id.
removeInplacePackages

@wz1000
Copy link
Collaborator

wz1000 commented Feb 24, 2021

OK. However, this change will mean that the hiedb database will have a lot of duplicate/redundant keys which will show up in queries. I think we will need to write a migration script for this - the easiest thing to do would be to just delete all the modules from "fake_uid", and letting ghcide take care of re-indexing. Another easy change could be to just bump sCHEMA_VERSION in hiedb, which will force ghcide to delete the old database and re-index everything.

See https://github.com/wz1000/HieDb/blob/62e24fb2b10ed17fadb0b1c69fc1067b6fc2984e/src/HieDb/Create.hs#L264 and https://github.com/wz1000/HieDb/blob/62e24fb2b10ed17fadb0b1c69fc1067b6fc2984e/src/HieDb/Create.hs#L38 respectively.

@pepeiborra
Copy link
Collaborator Author

How about adding a version number to the path computed by getHieDbLoc? The old database will stay around, but that's not a bad thing - it will still work for HLS 1.0.0.0

@pepeiborra
Copy link
Collaborator Author

@wz1000 I have added the version number to the hiedb path

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

Successfully merging this pull request may close these issues.

Program error: Failed to load interface for ‘Main’ no unit id matching ‘main’ was found
3 participants