Skip to content

Handle absolute as well as relative paths (issue #969) #1317

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Aug 30, 2018

Conversation

SethPoulsen
Copy link
Contributor

Same as pull request #1321 but comparing to horizon now. That really tested my git skills. Glad I got it figured out.

@jpolitz
Copy link
Member

jpolitz commented Apr 8, 2018

In the other discussion, @SethPoulsen said something which I think is useful here:

If you want everything in reference to the build directory internally, rather than allowing absolute and relative paths, we could just change the get-real-path function I created to convert all absolute paths to relative paths starting at the build directory.

I think this is likely the right way to go. At the moment, all the built phase0 files have mkolosick in them; in general we could push that to CI and then they'd have /home/travis in them. I think it makes much more sense, if we're going to canonicalize on either of these options, to do so relative to the build directory.

@blerner
Copy link
Member

blerner commented Apr 8, 2018

Agreed. (Though I'd like to potentially go further yet: I don't think we ought to preserve any actual directory names in the built output at all. I think we could anonymize the directories we encounter, as "a", "b", "c", "d', "a/a", "a/b", etc, thereby getting consistent builds across different people's machines, as long as their directory structures were merely isomorphic...)

@jpolitz
Copy link
Member

jpolitz commented Apr 8, 2018

Does that need to be part of this PR?

@blerner
Copy link
Member

blerner commented Apr 8, 2018

Not necessarily; I don't think your comment needs to be part of this PR either, per se. :) This just gets us to supporting other paths better, and a separate PR could normalize the paths however we choose.

@SethPoulsen
Copy link
Contributor Author

I've changed the get-real-path function to return every path as relative.

However, there are still things like "uri":"file:///home/seth/pyret/pyret-lang/src/arr/compiler/js-ast.arr" lying around in the pyret.jarr, even for phase B and C. I'm not sure why this is, I don't think I understand how the whole bootstrap works well enough yet.

@blerner What's the benefit of getting rid of path names altogether? It seems kind of nice to have things like src/arr/compiler... lying around so that we can see where files came from.

@SethPoulsen
Copy link
Contributor Author

Have there been any decisions made yet as to what we want to do here?

@jpolitz
Copy link
Member

jpolitz commented Aug 29, 2018

IMO we should just merge this since folks try it and its useful.

The only reason I have hesitated on absolute paths is that if we decide to go to a system where we copy the directory structure of the source tree to a target tree, then absolutes are gnarly to handle.

But given the way the compiler and locators work right now, there's no reason to not support this that I can see, and I think relative paths will have all the same issues.

@SethPoulsen
Copy link
Contributor Author

Sounds good to me.

What is the argument for copying to a target tree?

@jpolitz
Copy link
Member

jpolitz commented Aug 30, 2018

The main argument for copying to a target tree is interfacing with vanilla JS files that use built-in node require. It's pretty reasonable to want to call some JS-native functions from Pyret, and Pyret supports it with import js-file(...) as ...

Though this works for many simple cases, since all Pyret modules are copied into a single directory layer with hashed names, they can't reliably use relative path imports if they depend on other plain JavaScript files. However, if the compiler copied the directory structure of the compiled source, then any JS dependencies could be copied over with the same relative structure, and imports would Just Work. This also plays nicely with tools like webpack and browserify.

Generally, it would make the compiled output of Pyret feel more like a regular JavaScript project instead of a language-specific cache.

@jpolitz jpolitz merged commit 0e23826 into brownplt:horizon Aug 30, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants