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

Deduplicate config file path calculation in test. #7387

Closed
wants to merge 1 commit into from

Conversation

athas
Copy link
Collaborator

@athas athas commented May 9, 2021

Tested with cabal test Cabal-tests:hackage-tests (man does Cabal have many test suites).


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

Now looking at this in own context i see that this is not good. defaultConfigFile doesn't consider $CABAL_CONFIG environment variable, so that is wrong.

Perfectly there should be a function in cabal-install to find the config file (c.f. https://hackage.haskell.org/package/cabal-install-parsers-0.4.1/docs/src/Cabal.Config.html#findConfig) but AFAICS it's backed into loadRawConfig (which is not bad, you probably want to read the config - as in here).

Yet, adding cabal-install lib as dependency to Cabal-tests doesn't feel right. There is "configuration parsing library" screaming to get out, whole cabal-install is big.

So, either use loadRawConfig to not duplicate any parsing logic, or let's drop this change.

@athas
Copy link
Collaborator Author

athas commented May 9, 2021

Now looking at this in own context i see that this is not good. defaultConfigFile doesn't consider $CABAL_CONFIG environment variable, so that is wrong.

defaultConfigFile uses getCabalDir, which looks up the CABAL_DIR environment variable AFAICS.

I guess one could use loadRawConfig, but the types involved are completely different, so I have no idea what that would actually entail.

@athas
Copy link
Collaborator Author

athas commented May 9, 2021

Oh, right, CABAL_CONFIG... well, is that needed for this test? I'm not even completely sure what it is testing.

@athas athas force-pushed the reduce-path-duplication branch from a6b0d4c to 7739e62 Compare May 9, 2021 09:34
@athas
Copy link
Collaborator Author

athas commented May 9, 2021

Now it uses getConfigFilePath, which respects CABAL_CONFIG. I don't see a realistic way to get rid of the cabal-install dependency.

@phadej
Copy link
Collaborator

phadej commented May 9, 2021

I'm not even completely sure what it is testing.

hackage-tests tests that whole Hackage's 01-index.tar can be parsed (when we do .cabal file parser changes). So it has to find 01-index.tar.

@athas athas closed this Jun 25, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Jun 25, 2021

@athas: do you need help with this PR or a future incarnation of it? Please shout to me, if so.

@athas
Copy link
Collaborator Author

athas commented Jun 25, 2021

I kind of lost motivation to do anything with it, since I don't fully understand the problem or the solution. Maybe it will become necessary to address as I do some more work on #7386 (particularly when I implement the CABAL_HOME fallback behaviour).

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.

3 participants