Skip to content

Conversation

@ericmj
Copy link
Member

@ericmj ericmj commented Jun 3, 2016

No description provided.

@josevalim
Copy link
Member

We should probably do this when we build the records and not as a post
processing step later on.

On Friday, June 3, 2016, Eric Meadows-Jönsson notifications@github.com
wrote:


You can view, comment on, or merge this pull request online at:

#4767
Commit Summary

  • Ensure paths stored in manifest are relative

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4767, or mute the thread
https://github.com/notifications/unsubscribe/AAAlbsM8_xjVgDsJ7NwYKikCgpV8fH2jks5qH_xUgaJpZM4ItYo6
.

José Valim
www.plataformatec.com.br
Skype: jv.ptec
Founder and Director of R&D

@ericmj
Copy link
Member Author

ericmj commented Jun 3, 2016

@josevalim We already seem to do that (in 1.2.5 also). But that raises the question where the absolute paths are coming from.

@josevalim
Copy link
Member

@ericmj so we should likely answer that question before merging this. :)

@ericmj ericmj force-pushed the emj-manifest-relative branch from 1905489 to e475a27 Compare June 4, 2016 19:08
@ericmj
Copy link
Member Author

ericmj commented Jun 4, 2016

Found the root cause; for dependencies the relative path does not work since _build/ is relative from the project directory - not from deps/my_dep.

The fix is to only store the filename in the manifest and build the absolute path when reading the manifest file.

Copy link
Member

Choose a reason for hiding this comment

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

There is a TODO!

Copy link
Member

Choose a reason for hiding this comment

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

That you didn't do!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, that's embarrassing.

@ericmj ericmj force-pushed the emj-manifest-relative branch from e475a27 to 62cf2c8 Compare June 4, 2016 19:30
@ericmj
Copy link
Member Author

ericmj commented Jun 4, 2016

Pushed with fixes based on your comments.

@josevalim
Copy link
Member

:shipit:

@josevalim josevalim merged commit f957d2f into master Jun 4, 2016
@josevalim josevalim deleted the emj-manifest-relative branch June 4, 2016 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants