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

dhall resolve doesn't respect relative directories #617

Closed
FintanH opened this issue Oct 3, 2018 · 19 comments · Fixed by #619
Closed

dhall resolve doesn't respect relative directories #617

FintanH opened this issue Oct 3, 2018 · 19 comments · Fixed by #619
Labels

Comments

@FintanH
Copy link
Collaborator

FintanH commented Oct 3, 2018

Working off of d1e8fde

Directory structure:

.
└── test
    ├── dub
    └── num

dub

{ dub : Double }

num

{ num : ./dub }

Error

Running dhall freeze --inplace "./test/num" the following error is produced:

↳ ./dub

Error: Missing file /home/fintan/Developer/test-freeze/dub

EDIT

Actually this isn't really freezes fault. This is down to resolution.
Running cat ./test/num | dhall resolve we get the same error as above

@FintanH FintanH added the bug label Oct 3, 2018
@f-f
Copy link
Member

f-f commented Oct 3, 2018

Oh, thank you for opening this 👏
I discovered roughly one month ago that we have the same issue with dhall-kubernetes (e.g. this file) but I totally forgot to open an issue about it.

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

@f-f Heh, for sure! I'm gonna try fix it now. We want to get caching working so first step is to get freezing working 😁

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

Seems that the import is using Local Here for some reason:

Import {importHashed = ImportHashed {hash = Nothing, importType = Local Here (File {directory = Directory {components = []}, file = "dub"})}, importMode = Code}

@FintanH FintanH changed the title dhall freeze doesn't respect relative directories dhall resolve doesn't respect relative directories Oct 3, 2018
@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

It seems that loadWith doesn't realize we've gone an extra directory down the tree.

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

I'm starting to realize the underlying issue here. So when we do cat ./test/num | dhall resolve the cat turns the expression into { dub : ./dub }. Then dhall resolve sees ./dub and assumes it's in the same directory as the call. This isn't the case so we've lost information. I think this is the correct behavior. The INCORRECT behavior is loading the file when doing the freeze so I'll try go down that route to fix this 😄 #rubberducking

@f-f
Copy link
Member

f-f commented Oct 3, 2018

@FintanH this is my reproduction case:

 $ dhall freeze --inplace examples/deployment.dhall

↳ ./../api/Deployment/default

Error: Missing file /Users/fabrizio/code/dhall-kubernetes/../api/Deployment/default

Where this is the file I'm trying to freeze, and it imports the file ../api/Deployment/default on the second line.

Note that if you do dhall <<< "./examples/deployment.dhall" then all is well.

EDIT: of course if you do cd examples && dhall freeze --inplace deployment.dhall then it works properly.

So the issue is the same one that you're describing in your last comment: what freeze is doing here is not right

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

@f-f Can you try use https://github.com/FintanH/dhall-haskell/tree/fintan/fix-freeze and see if it works for you? Make sure to have a copy just in case 😬 Also it seems it needs to be ./example/deployment.dhall. Need to figure out why it needs that relativity

@f-f
Copy link
Member

f-f commented Oct 3, 2018

@FintanH tried the branch:

  • if I don't specify the ./, it nukes the file
  • if I do specify it, then it replaces the content with an import to itself, protected by some sha 😄

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

I think the second point is actually what we want, right? Because if you run it through dhall it loads it from the cache. @Gabriel439 can you confirm?

@f-f
Copy link
Member

f-f commented Oct 3, 2018

No, that would lose whatever content is in the file, so I cannot edit it anymore, ever.
What freeze should do is: replace any import inside a file with the same import with a hash attached to it (this is also the current behaviour on master)

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

Ahhhh right. I'll get on it 😂

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

I need some way of forcing only one level of resolution so that I can then walk the imports in the input file 🤔

@Gabriella439
Copy link
Collaborator

@FintanH: In the Haskell REPL you can do traverse exprFromImport since an Expr is Traversable

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

Thanks, I'll give it a look tomorrow 👌

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 3, 2018

I was taking a look because of obsessive brain. I'm not sure I can come up with a solution using what's already there. I think some functionality akin to the Dhall.Import functions might be needed.

Gabriella439 added a commit that referenced this issue Oct 4, 2018
Fixes #617

This changes `dhall freeze` to use the directory of the supplied file path as
the root import instead of using ".".  This fixes the behavior of relative
imports when the root file is not located in the current working directory.
@Gabriella439
Copy link
Collaborator

Fix up here: #619

@Gabriella439
Copy link
Collaborator

Also, note that this is not an issue with import resolution per se. For example, dhall < ./foo/bar has intentionally never correctly resolved imports relative to ./foo/bar correctly because it can't know that standard input came from that file path (or that it even came from a file at all). This is why you always should do dhall <<< './foo/bar' instead. See:

https://github.com/dhall-lang/dhall-lang/wiki/Frequently-Asked-Questions-(FAQ)#imports-relative-to-my-top-level-file-are-not-working

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 4, 2018

Ya, I realized this after the face 😳 Thanks for the fix!

Gabriella439 added a commit that referenced this issue Oct 4, 2018
Fixes #617

This changes `dhall freeze` to use the directory of the supplied file path as
the root import instead of using ".".  This fixes the behavior of relative
imports when the root file is not located in the current working directory.
@Gabriella439
Copy link
Collaborator

You're welcome!

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

Successfully merging a pull request may close this issue.

3 participants