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

allow for spaces in paths for windows #119

Merged
merged 4 commits into from
Oct 30, 2021
Merged

allow for spaces in paths for windows #119

merged 4 commits into from
Oct 30, 2021

Conversation

simonmcconnell
Copy link
Contributor

fixes #118
type command fails silently, resulting in an empty executable

@jjcarstens
Copy link
Contributor

Thanks @simonmcconnell!

I'm hesitant to merge this because type was introduced recently to fix a different issue in compiling the CPIO on Windows. So it seems like this will fix one specific case, but break the others

What it also seems like is that Windows has quite an extensive array of environment setups that really affect compilation of the tools and resulting executable but our simple :os.type checks aren't specific enough in those cases.

I don't know enough about Windows to make a decision here so I would love feedback if you have an idea how best to handle these env differences. Maybe if its windows we System.find_executable("type") and if thats nil, we try cat?

@simonmcconnell
Copy link
Contributor Author

I believe it was me that changed cat to type originally. Looking into the empty exe issue this morning I could not get type to work for the paths output by bakeware, but I think it it just the / in the path.

You're right, if you run type in mingw you get a different type to the one in the command prompt. I'll have to look into it further when I've got a spare few hours. Maybe I was too quick on the trigger with this PR because I introduced the bug 🤭.

@jjcarstens
Copy link
Contributor

@simonmcconnell Whats the status of this? Is there more investigation to be done?

@simonmcconnell
Copy link
Contributor Author

@jjcarstens I tested it on git bash, PowerShell and CMD prompt so I'm happy.

Do you think it's worth adding the three different shells to CI? If that's even possible.

@jjcarstens
Copy link
Contributor

O absolutely - If its even possible (and I'm not doing it 😉 ) then I would love all 3 in CI

@simonmcconnell
Copy link
Contributor Author

Ya bloody slacker! This is open source therefore you do everything free 😉

I'll play with the CI options in a test repository and report back.

@simonmcconnell simonmcconnell changed the title revert to cat for windows allow for spaces in paths for windows Oct 30, 2021
@jjcarstens
Copy link
Contributor

Thanks! I think we'll go ahead and merge this and I'll make a release in a bit.

If you (or someone) happens to get that CI magic, we'll put that in later

@jjcarstens jjcarstens merged commit 7146ad6 into bake-bake-bake:main Oct 30, 2021
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.

Windows executable is an empty file
2 participants