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

Fix separate make and install execution error issue. #8540

Merged
merged 3 commits into from Oct 22, 2019

Conversation

JackBoosY
Copy link
Contributor

The install command has not been executed, it looks like it was caused by the separation of make and install.

Related: #8473.

@JackBoosY JackBoosY added the info:internal This PR or Issue was filed by the vcpkg team. label Oct 10, 2019
@JackBoosY
Copy link
Contributor Author

@Christsnatcher @spindensity @oracleoftroy @timautry
Could you test it?
Thanks.

@spindensity
Copy link

spindensity commented Oct 10, 2019

@JackBoosY

I've tested it. It works perfectly.

Thanks.

@oracleoftroy
Copy link
Contributor

To be honest, I'm a bit surprised it worked, but it worked for me as well.

I don't know what vcpkg's policy is for making sure vcpkg still works if installed in paths with spaces, but I suspect the change on line 55 of vcpkg_build_make.cmake to remove the quotes around "${_VCPKG_PROJECT_SUBPATH}make" would break this use case. But if those quotes are contributing to this error in the first place, then I vote that they go.

@JackBoosY
Copy link
Contributor Author

@oracleoftroy Well, I think this is due to the simultaneous use of cmake_parse_arguments and execute_process. I don't know what cmake does in the command for the arguments behind the command.

@oracleoftroy
Copy link
Contributor

Hmm, I see. I wish I understood cmake and vcpkg internals better. I got stuck at this part. It just surprised me that a few quotes added and removed was all it took. :)

@Christsnatcher
Copy link

Thank you @JackBoosY, working fine here either.

@Neumann-A
Copy link
Contributor

@JackBoosY
The difference in cmake is:
"i am not a list but a single element"
i am a list
"i;am;a;list"

@Christsnatcher
Copy link

@Neumann-A: The removal of these particular quotes apparently isn't necessary, I just tried and x264 building worked flawlessly with the quotes in place.

@JackBoosY JackBoosY marked this pull request as ready for review October 10, 2019 14:37
@oracleoftroy
Copy link
Contributor

@JackBoosY I took a look at the quotes on line 55 this morning and I can confirm what @Christsnatcher says. I added quotes around just the make executable, changing line 55 to:

set(MAKE ${BASH} --noprofile --norc -c "${_VCPKG_PROJECT_SUBPATH}make")

I had no issue installing x264 with this change.

@vicroms vicroms self-assigned this Oct 10, 2019
@Neumann-A
Copy link
Contributor

@JackBoosY: Got this error in WSL with this PR merged into my libpq PR:

CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:136 (message):
    Command failed: "make -j" 9
    Working Directory: /mnt/d/vcpkg_test/libpq_port/buildtrees/libpq/x64-linux-dbg
    See logs for more information:

@Neumann-A
Copy link
Contributor

Solved it with COMMAND "${MAKE};${MAKE_OPTS}" but this will brake your windows path. The problem is you are building the command wrongly. You want to build the bash and make command separatly. You then need to call excute_build_process with a list of commands in relation to the bash command and have the make command be passed as a single string to the bash command.

@JackBoosY
Copy link
Contributor Author

@Neumann-A I don't understand the difference between command and parameter splitting between windows and linux, but it is successful with your workaround.

Good work!

@JackBoosY JackBoosY mentioned this pull request Oct 22, 2019
@vicroms vicroms merged commit 1bdb6bf into microsoft:master Oct 22, 2019
@JackBoosY JackBoosY deleted the dev/jack/8473 branch October 22, 2019 07:26
@ghost
Copy link

ghost commented Oct 22, 2019

Thank you @JackBoosY - does the job on both VS2017 and VS2019 latest builds. I appreciate the hard work you put in on this. Have a wonderful week.

@Christsnatcher
Copy link

Working perfectly now, a huge "Thank you" to everyone involved in fixing this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants