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

Dont resolve symlinks in cradle discovery #219

Merged
merged 2 commits into from
Jul 7, 2020
Merged

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jul 2, 2020

Prefer 'makeAbsolute' over 'canonicalizePath'

Solves the problem described in #218 but I am not sure if it would break something

@fendor fendor changed the title Dont resolve symlinks in cradle initilisation Dont resolve symlinks in cradle discovery Jul 2, 2020
@fendor fendor requested a review from jneira July 2, 2020 13:15
@fendor
Copy link
Collaborator Author

fendor commented Jul 2, 2020

cc @bubba would you mind to give a review here as well?

Copy link
Contributor

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

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

I don't really understand how canonicalising the path affected the paths in that issue but yeah if we can get away with using makeAbsolute then we should. We should probably add a test case for this

@fendor
Copy link
Collaborator Author

fendor commented Jul 2, 2020

@bubba canonicalizePaths resolves symlinks, which caused the issue #219. with makeAbsolute, we only prepend the current directory and normalise it, remove indirections, etc... but do not resolve symlinks. I am not sure what could break from it and how to add a proper test-case for symlinks

@lukel97
Copy link
Contributor

lukel97 commented Jul 2, 2020

Ah, I skimmed the last crucial sentence of that issue. You can add symbolic links in git (last time I checked), so maybe just a folder like

- hie.yaml
- foo/ -> bar/
- bar/

Where hie.yaml is

cradle:
    multi:
        - path: "./foo"
          config: { cradle: { default:  } }

And then test that you can get the cradle for /foo/A.hs

@fendor fendor force-pushed the issue/218 branch 2 times, most recently from f8f32d4 to 18e2402 Compare July 2, 2020 18:48
@fendor
Copy link
Collaborator Author

fendor commented Jul 2, 2020

Added failing test-cases. I decided to solve it programmatically, since I can not add this symlink to the sdist tar archive (or at least, I expect troubles and pain).

I verified that the test-case used to fail and doesnt anymore.

@fendor fendor force-pushed the issue/218 branch 2 times, most recently from 7259b6f to eb40274 Compare July 7, 2020 14:06
@fendor fendor merged commit b925bbf into haskell:master Jul 7, 2020
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.

2 participants