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

Do not try to symlink libclang on Windows #914

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

gokhanettin
Copy link
Contributor

@gokhanettin gokhanettin commented Jan 27, 2018

create_symlink is UNIX specific1, and it breaks the build
on Windows.


This change is Reviewable

create_symlink is UNIX specific[1], and it breaks the build
on Windows.

[1]: https://cmake.org/cmake/help/v3.9/manual/cmake.1.html
@bstaletic
Copy link
Collaborator

What's the error this PR is trying to fix? We have build bots constantly building ycmd on Windows and it didn't break once.

@puremourning
Copy link
Member

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


cpp/ycm/CMakeLists.txt, line 412 at r1 (raw file):

    )

    if( NOT APPLE AND NOT WIN32 )

This line is in an else vs if (MSVC). What compiler are you using ?


Comments from Reviewable

@gokhanettin
Copy link
Contributor Author

cpp/ycm/CMakeLists.txt, line 412 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This line is in an else vs if (MSVC). What compiler are you using ?

After some more modifications I manged to build with MINGW. I have not tried with MSVC. The error was a failure to create the symlink. I have just noticedif (MSVC). It explains why the build bot does not fail, but it would I believe with any other compiler on Windows. I am not sure about cygwin though. May be if (MSVC) should be if(WIN32) or NOT WIN32 should be NOT MINGW in my PR.


Comments from Reviewable

@gokhanettin
Copy link
Contributor Author

I am building with python installer.py --clang-completer --system-libclang using MINGW64.
This PR prevents the code calling cmake.exe -E create_symlink when compiler is not MSVC on Windows.
Here is the error message:

C:\msys64\mingw64\bin\cmake.exe -E create_symlink libclang.dll.a C:/msys64/home/gokhanettin/.config/nvim/plugged/YouCompleteMe/third_party/ycmd/libclang.dll.a""
failed to create symbolic link 'C:/msys64/home/gokhanettin/.config/nvim/plugged/YouCompleteMe/third_party/ycmd/libclang.dll.a': No error
ninja: build stopped: subcommand failed.
Searching Python 2.7 libraries...
Found Python library: C:/msys64/mingw64/lib/python2.7/config/libpython2.7.dll.a
Found Python headers folder: C:/msys64/mingw64/include/python2.7
ERROR: the build failed.

Comments from Reviewable

@codecov-io
Copy link

codecov-io commented Jan 27, 2018

Codecov Report

Merging #914 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #914   +/-   ##
======================================
  Coverage    96.2%   96.2%           
======================================
  Files          83      83           
  Lines        6500    6500           
======================================
  Hits         6253    6253           
  Misses        247     247

@micbou
Copy link
Collaborator

micbou commented Jan 30, 2018

The symlink is only needed on Linux so it makes sense to exclude Windows (which is going to be MinGW or MSYS in that scenario).

:lgtm:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

:lgtm:

Let's merge this. @zzbot r=micbou


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Jan 30, 2018

📌 Commit 692cfdb has been approved by micbou

@zzbot
Copy link
Contributor

zzbot commented Jan 30, 2018

⌛ Testing commit 692cfdb with merge a54135f...

zzbot added a commit that referenced this pull request Jan 30, 2018
Do not try to symlink libclang on Windows

create_symlink is UNIX specific[1], and it breaks the build
on Windows.

[1]: https://cmake.org/cmake/help/v3.9/manual/cmake.1.html

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/914)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Jan 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing a54135f to master...

@zzbot zzbot merged commit 692cfdb into ycm-core:master Jan 30, 2018
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Feb 10, 2018
[READY] Update ycmd

Include the following changes:

 - PR ycm-core/ycmd#789: add support for Windows flags when --driver-mode=cl is given;
 - PR ycm-core/ycmd#848: hide C++ symbols by default;
 - PR ycm-core/ycmd#857: add Java support using jdt.ls;
 - PR ycm-core/ycmd#861: translate libclang error codes to exceptions;
 - PR ycm-core/ycmd#880: support downloading Clang binaries on ARM systems;
 - PR ycm-core/ycmd#883: handle zero column diagnostic from OmniSharp;
 - PR ycm-core/ycmd#884: specify Platform property when compiling OmniSharp;
 - PR ycm-core/ycmd#886: use current working directory in JavaScript completer;
 - PR ycm-core/ycmd#887: update Boost to 1.66.0;
 - PR ycm-core/ycmd#888: update JediHTTP;
 - PR ycm-core/ycmd#889: update Clang to 5.0.1;
 - PR ycm-core/ycmd#891: fix building with system libclang on Gentoo amd64;
 - PR ycm-core/ycmd#904: drop Python 2.6 and Python 3.3 support;
 - PR ycm-core/ycmd#905: calculate the start column when items are not resolved in the language server completer;
 - PR ycm-core/ycmd#912: download Clang binaries from HTTPS;
 - PR ycm-core/ycmd#914: do not try to symlink libclang on Windows.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2902)
<!-- Reviewable:end -->
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.

6 participants