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.

Sign up for GitHub

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

Jump to bottom

trunk 0.8.0 fails to build if the target directory does not already exist #124

Closed
jbg opened this issue Feb 4, 2021 · 8 comments · Fixed by #125 or #129
Closed

trunk 0.8.0 fails to build if the target directory does not already exist #124

jbg opened this issue Feb 4, 2021 · 8 comments · Fixed by #125 or #129
Labels
bug Something isn't working cli Trunk CLI application

Comments

@jbg
Copy link
Contributor

jbg commented Feb 4, 2021

To reproduce:

rm -rf target
trunk build

Expected: trunk behaves as cargo does and creates the necessary target directories.

Actual: trunk crashes with a cryptic error:

  ❌ trunk | error
Error: No such file or directory (os error 2)

but the cause can be discovered using strace:

lstat("/redacted/target", 0xredacted) = -1 ENOENT (No such file or directory)
@rakshith-ravi rakshith-ravi added bug Something isn't working cli Trunk CLI application labels Feb 4, 2021
@jbg
Copy link
Contributor Author

jbg commented Feb 4, 2021 •
edited
Loading

I think this might be the culprit: https://github.com/thedodd/trunk/blob/master/src/config/manifest.rs#L25

Path::canonicalize() returns an error if the path doesn't exist.

@thedodd
Copy link
Member

thedodd commented Feb 4, 2021

@jbg cryptic errors like that are the worst! I thought we got rid of all of them. Your analysis may indeed be correct. I'll repro and verify a fix. We should be able to get a 0.8.1 fix out pretty quickly.

@thedodd
Copy link
Member

thedodd commented Feb 4, 2021

@jbg you were indeed correct. Turns out the cargo metadata already returns that path as a canonical path, so there was no need for us to be attempting to canonicalize it to begin with. I've made the fix, and verified that it does indeed address the issue. Thanks for the report!

thedodd added a commit that referenced this issue Feb 4, 2021
@thedodd
Fix a bug related to path canonicalization.
9f1d45c 
closes #124
@thedodd thedodd mentioned this issue Feb 4, 2021
Merged
2 tasks
@thedodd
Copy link
Member

thedodd commented Feb 4, 2021

v0.8.1 is on its way out right now.

@tommket
Copy link
Contributor

tommket commented Feb 10, 2021

Sorry to chime in with the bad news, but the fix in v0.8.1 apparently does not work for the Yew example.
Steps to reproduce: cd into the examples/yew directory and execute the trunk serve --open as stated in the readme.

@tommket tommket mentioned this issue Feb 10, 2021
Merged
@thedodd
Copy link
Member

thedodd commented Feb 10, 2021

@tommket that is actually what I did to verify that 0.8.1 actually did indeed resolve the issue. Let me try again, it is always possible that I messed up. Do you mind just also double-checking and making sure that you are indeed on 0.8.1 and not 0.8.0?

@thedodd
Copy link
Member

thedodd commented Feb 10, 2021

Alas, it appears as though you are correct. I apparently did not verify the fix as thoroughly as I thought. I will re-open and fix ASAP.

@thedodd thedodd reopened this Feb 10, 2021
thedodd added a commit that referenced this issue Feb 10, 2021
@thedodd
Round two of fixing #124
da9b1fb 
This time, the path was being canonicalized in another location. My
original verification apparently did not account for this.

All calls to `canonicalize` have been audited, and every location where
such is called, it is also expected that the path exist, and is indeed
considered an error if it does not exist.
thedodd added a commit that referenced this issue Feb 11, 2021
@thedodd
Round two of fixing #124
a1b3ccb 
This time, the path was being canonicalized in another location. My
original verification apparently did not account for this.

All calls to `canonicalize` have been audited, and every location where
such is called, it is also expected that the path exist, and is indeed
considered an error if it does not exist.
@thedodd thedodd mentioned this issue Feb 11, 2021
Merged
2 tasks
thedodd added a commit that referenced this issue Feb 14, 2021
@thedodd
Round two of fixing #124
0a6bd27 
This time, the path was being canonicalized in another location. My
original verification apparently did not account for this.

All calls to `canonicalize` have been audited, and every location where
such is called, it is also expected that the path exist, and is indeed
considered an error if it does not exist.
thedodd added a commit that referenced this issue Feb 14, 2021
@thedodd
Round two of fixing #124
3c65df6 
This time, the path was being canonicalized in another location. My
original verification apparently did not account for this.

All calls to `canonicalize` have been audited, and every location where
such is called, it is also expected that the path exist, and is indeed
considered an error if it does not exist.
thedodd added a commit that referenced this issue Feb 14, 2021
@thedodd
Merge pull request #129 from thedodd/124-round-two
6dc36e7 
Round two of fixing #124
@thedodd
Copy link
Member

thedodd commented Feb 16, 2021

@tommket && @jbg I've just released v0.8.2. Release binaries are available on GH releases page & on Brew. Hopefully we don't go to a round 3. To be sure, I've audited every call to canonicalize in the code base, and there are no more cases where this should arise.

Thanks for you patience! I got sick on Friday 12th, otherwise I would have released these changes earlier. Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working cli Trunk CLI application
Projects
None yet
Milestone
No milestone
4 participants
@jbg @thedodd @rakshith-ravi @tommket