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

Fixing duplicate absolute install paths when using Visual Studio generators #639

Merged
merged 2 commits into from Jun 7, 2019

Conversation

RikoOphorst
Copy link
Contributor

A slight bug occurs when running a cmake install on shaderc when using Visual Studio as a generator, where the source install file paths are incorrect. They contain a duplicate absolute path.

This issue occurred when a pull request was merged in order to fix the Ninja build system back in this PR #570

The issue is fixed by relying on the shaderc_combine_static_lib() function to properly set an absolute path on the location of the shaderc_combined custom target.

You can verify that this install behavior now works properly by running this on a Windows machine, generating for VS2017:

git clone https://github.com/RikoOphorst/shaderc
cd shaderc
mkdir build
cd build
cmake ../ -G"Visual Studio 15 2017 Win64"
cmake --build . --target install

Apologies if I'm submitting this pull request incorrectly, this is the first time I've made a pull request in any repo.

… another path which already is a correct absolute path, resulting in two absolute paths concatenated together
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@RikoOphorst
Copy link
Contributor Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@zoddicus
Copy link
Collaborator

zoddicus commented Jun 3, 2019

Adding @s-perron and @fjhenigman as reviewers, since they were involved in the referenced PR.

@noct
Copy link

noct commented Jun 3, 2019

The same change should also be applied to libshaderc_spvc\CMakeLists.txt, as that fails for the same reason.

@RikoOphorst
Copy link
Contributor Author

@noct Should be fixed!

@dneto0
Copy link
Collaborator

dneto0 commented Jun 4, 2019

The CI-ndk-build failure looks like a flake. Will try again.

@RikoOphorst
Copy link
Contributor Author

@dneto0 I'm not sure what's happening with the NDK build. Could you elaborate on what steps I need to take?

@dneto0
Copy link
Collaborator

dneto0 commented Jun 5, 2019

@dneto0 I'm not sure what's happening with the NDK build. Could you elaborate on what steps I need to take?

No action at this time. It failed mysteriously again, about when the script downloads the ninja executable. It appears wget fails with exit code 8.... ? I haven't reproduced locally. I'll try kokoro-run again.

++ dirname github/shaderc/kokoro/ndk-build/build_khronos.sh
+ SCRIPT_DIR=github/shaderc/kokoro/ndk-build
+ source github/shaderc/kokoro/ndk-build/build.sh
++ set -e
++ set -x
++ BUILD_ROOT=/tmpfs/src
++ SRC=/tmpfs/src/github/shaderc
++ wget -q https://github.com/ninja-build/ninja/releases/download/v1.7.2/ninja-linux.zip
[ID: 8884006] Build finished after 2 secs, exit value: 8

@dneto0
Copy link
Collaborator

dneto0 commented Jun 5, 2019

oh, it looks like the second kokoro run didn't occur... I'l have to see how to kick that off again.
Hey @ehsannas can you help?

@ehsannas
Copy link
Collaborator

ehsannas commented Jun 5, 2019

hmm.. looks like the kokoro:run label is not working today... I'll kick them off manually.

@ehsannas
Copy link
Collaborator

ehsannas commented Jun 5, 2019

Jobs restarted

@RikoOphorst
Copy link
Contributor Author

@ehsannas @dneto0 @noct Seems the checks have passed successfully! Anything else I need to do, or do I just wait now?

@dneto0 dneto0 merged commit dc67137 into google:master Jun 7, 2019
@dneto0
Copy link
Collaborator

dneto0 commented Jun 7, 2019

Thank you @RikoOphorst for your contribution and your patience.

dneto0 added a commit to dneto0/shaderc that referenced this pull request Nov 14, 2019
Including:
ef2b7a6 Avoid C-style cast for constants definitions (google#760)
c1af9a7 SetBeforeHlslLegalization to use more relaxed validation rules (google#676)
72988e4 Explicitly use python3 for git-sync-deps (google#759)
1f2dec1 Remove app_dummy call in Android test (google#756)
e9bb8f2 Add verbose flag to run_spirv_cross_tests.py (google#753)
f396afa Rolling 4 dependencies and update known_failures (google#755)
77d7b65 Rolling 4 dependencies and updating known_failures (google#754)
23bbb32 Add --target-spv option to set target SPIR-V version (google#750)
8de4816 Remove old logging infra from run_spirv_cross_tests.py (google#749)
30d8262 Enable implicit fallthrough warning and fix examples of (google#747)
7781794 Rolling 2 dependencies (google#748)
48e07b5 Rolling 4 dependencies and update known_failures (google#745)
cff2a56 Catch low level exceptions instead of crash (google#744)
11cf0a3 Add jobs flag to spirv cross tests script (google#743)
0ce3725 Extract common test environment bits out into a class (google#738)
c4ba097 Fix BUILD.gn for Chromium build. (google#741)
ab08017 BUILD.gn fixes for Fuchsia. (google#733)
b9a1021 Remove Appveyor artifacts link. (google#739)
16f6e6a Remove SetShaderModel from API (google#734)
b3523d5 Move script fixes from other SPIRV related repos (google#732)
a731ee3 Import improvements to DEPS roll script (google#729)
213a2a8 Update CHANGES (google#730)
2cbf790 Add -fnan-clamp (google#725)
6c4c6ed Add script to generate manual roll patch (google#727)
4714ba1 Rolling 4 dependencies (google#728)
2a888de Roll third_party/glslang def9662348b0..9db72785beb3 (1 commits) (google#721)
daf804a Roll third_party/spirv-cross 4104e363005a..146dc453bcec (2 commits) (google#722)
9164aea Manually roll DEPS to get past test failure (google#717)
8b2600f Roll third_party/googletest fd20d1eccef6..176eccfb8f42 (1 commits) (google#718)
f40bcf8 Roll third_party/googletest e110929a7b49..fd20d1eccef6 (5 commits) (google#715)
8fbd7e0 Roll third_party/re2 0c95bcce2f1f..848dfb7e1d7b (2 commits) (google#712)
a0bfa18 Roll third_party/googletest f7c178ecb33c..e110929a7b49 (2 commits) (google#709)
7b84ab7 Roll third_party/googletest 26afdba792e5..f7c178ecb33c (2 commits) (google#707)
828e3a5 Roll third_party/spirv-cross 00a8539d1ddf..4104e363005a (3 commits) (google#698)
9745f0e Roll third_party/googletest af4c2cb098a3..26afdba792e5 (1 commits) (google#703)
c21dd10 Update glslc tests for -finvert-y option (google#704)
ceff3dd Pass InvertY to glslang (google#695)
b988e6b Roll third_party/googletest 89656ddbe62f..af4c2cb098a3 (2 commits) (google#702)
c6cb20d Roll third_party/spirv-headers 9cf7c3a7d2d2..de99d4d834ae (1 commits) (google#697)
bd60de4 Roll third_party/googletest 076b7f778883..89656ddbe62f (1 commits) (google#694)
dcfa9d0 Roll third_party/spirv-cross fccf1d046204..00a8539d1ddf (1 commits) (google#691)
ef6b960 Roll third_party/spirv-cross 5e9e8918f9a2..fccf1d046204 (1 commits) (google#690)
206a069 Roll third_party/spirv-headers 9242862c84fe..9cf7c3a7d2d2 (1 commits) (google#689)
2f42850 Roll all DEPS to HEAD (google#685)
facd092 Roll third_party/effcee 8f0a61dc95e0..b83b58d177b7 (2 commits) (google#677)
3c76b8a Re-introduce SetShaderModel for backwards compatibility (google#675)
8738101 Refactor dup'd arg handling functions into a single location (google#660)
f21bd30 Update Dockerfile to use Python 3 (google#668)
d0298a3  Add details of spvc to README.md (google#661)
78c188d Change TODO(name) to TODO(bug issue#) for spvc files (google#658)
dc67137 Fixing duplicate absolute install paths when using Visual Studio generators (google#639)
568cf20 Add in missing flags to pass more of the spirv-cross tests (google#664)
acf7d24 Convert vars['foo'] to Var('foo') (google#663)

Testing: checkbuild.py on Linux; unit tests on Windows

Change-Id: Ic91997e8266c6f53239373657fb1f566a0223bf9
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.

None yet

8 participants