Fixes to allow C code to compile with Visual C++ compiler #2061
Conversation
|
Code Climate has analyzed commit 22450ec and detected 0 issues on this pull request. View more on Code Climate. |
|
❌ Build nokogiri 1.0.630 failed (commit 42a63a789f by @preetpalS) |
|
@preetpalS Thanks for submitting this change. Please note that CI is failing: |
|
@preetpalS Hi! Can you provide some insight into why you're contributing these changes? I'd like to understand if you're addressing something that causes errors during compilation, or if these are just warnings, or something else. |
|
@flavorjones I am addressing something that causes errors during compilation. |
|
✅ Build nokogiri 1.0.631 completed (commit aba66fdb27 by @preetpalS) |
I compiled Ruby with the Microsoft Visual C++ compiler.I compiled it from the download from ruby-lang.org using the instructions in the win32 folder and by downloading and installing some libraries (like openssl, zlib) using vcpkg (you have to modify the The test suite runs but has some failures when running with Ruby compiled with the Visual C++ compiler (this may be due to issues with my setup though). I copied and pasted some the command line output from the compilation and test suite run below. Command Prompt Output |
|
@flavorjones This library cannot compile as-is when using Ruby compiled with the Visual C++ compiler. It's kind of a pain to install so here is some proof that these changes fix actual compilation errors: Log of compilation failures when not using these changes (first without using system libraries, then when using system libraries)C:\Users\preet\Projects\OpenSource\upstream>git clone https://github.com/sparklemotion/nokogiri.git C:\Users\preet\Projects\OpenSource\upstream>cd nokogiri C:\Users\preet\Projects\OpenSource\upstream\nokogiri>bundle C:\Users\preet\Projects\OpenSource\upstream\nokogiri>bundle exec rake compile IMPORTANT NOTICE: Building Nokogiri with a packaged version of zlib-1.2.11. Team Nokogiri will keep on doing their best to provide security If you are using Bundler, tell it to use the option: Downloading zlib-1.2.11.tar.gz (100%) IMPORTANT NOTICE: Building Nokogiri with a packaged version of libiconv-1.15. Team Nokogiri will keep on doing their best to provide security If you are using Bundler, tell it to use the option: Downloading libiconv-1.15.tar.gz (100%)
|
|
@preetpalS Thanks for all the logs! They give some more insight what's happening. Since the changes are non-obvious it's a good practice to copy+paste the error message of the particular compiler into the commit message like so: 368b354 This provides a direct relationship between the patch and the error it tries to fix. Regarding the test failures there is a serious encoding issue. I'm not sure where it comes from but it looks somehow like the Ruby build is broken in this regard. |
|
@larskanis Thanks for the tip, I should have done that for these commits, it would have made things more clear. The Ruby build I am using is broken (some more information on the what and why of this Ruby build and a log of running Ruby test suite with it).My usage of this Ruby build is mainly experimental. I was able to run a hello world Sinatra web app with the Puma web server (native gem) without any issues. I even got Rails working (I had to modify this gem along with the bcrypt gem; also bypassed the bootsnap gem; removed some ERB from a yaml file) but I ran into issues with the pg gem (I am guessing the SQLite3 adapter would compile though). The reason why I wanted to try this out was to see if I could use any of the CUDA-related gems on Windows. From what I understand, you need to use the Visual C++ compiler to work with CUDA on Windows. C:\ruby\builds\ruby-2.7.1\build> I am not sure if these commits should be accepted (I do not have much experience with C programming). If these changes do not do any harm, please consider them so that Nokogiri can be compiled when using a Ruby build that uses Visual C++ as its C compiler. If there is harm caused by these changes, maybe there is an opportunity to add a test to the test suite that could detect that harm. |
|
@preetpalS Thanks for the additional context. I don't have any philosophical objections to merging a change like this to ensure we can compile on your platform; but I'd like to ask that you please include the error message in the commit message, as Lars asked, particularly because we don't have automated testing set up for a Visual C++ compiler environment. |
Error was as follows: compiling ../../../../ext/nokogiri/xml_document.c xml_document.c C:\ruby\mswin64\ruby271\include\ruby-2.7.0\x64-mswin64_140\ruby/config.h(22): warning C4117: macro name '_INTEGRAL_MAX_BITS' is reserved, '#define' ignored ../../../../ext/nokogiri/xml_document.c(58): error C4028: formal parameter 1 different from declaration ../../../../ext/nokogiri/xml_document.c(58): error C4028: formal parameter 2 different from declaration ../../../../ext/nokogiri/xml_document.c(58): error C4028: formal parameter 3 different from declaration NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\cl.EXE"' : return code '0x2' Stop. rake aborted! Command failed with status (2): [nmake...]
Error was as follows: compiling ../../../../ext/nokogiri/xml_io.c xml_io.c C:\ruby\mswin64\ruby271\include\ruby-2.7.0\x64-mswin64_140\ruby/config.h(22): warning C4117: macro name '_INTEGRAL_MAX_BITS' is reserved, '#define' ignored ../../../../ext/nokogiri/xml_io.c(20): error C4028: formal parameter 1 different from declaration ../../../../ext/nokogiri/xml_io.c(20): warning C4113: 'VALUE (__cdecl *)(void)' differs in parameter lists from 'VALUE (__cdecl *)(VALUE,VALUE)' ../../../../ext/nokogiri/xml_io.c(47): error C4028: formal parameter 1 different from declaration ../../../../ext/nokogiri/xml_io.c(47): warning C4113: 'VALUE (__cdecl *)(void)' differs in parameter lists from 'VALUE (__cdecl *)(VALUE,VALUE)' NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\cl.EXE"' : return code '0x2' Stop. rake aborted! Command failed with status (2): [nmake...]
|
✅ Build nokogiri 1.0.632 completed (commit 937a971c56 by @preetpalS) |
|
I'm going to merge @larskanis's PR #2068 as I prefer that approach. Thank you so much for putting in the time to report this and to submit a proposed patch! I appreciate your efforts. |
Fix gcc warings - alternative to #2061
Thank you for contributing to Nokogiri! To help us prioritize, please take care to answer the questions below when you submit this pull request.
The Nokogiri core team work off of
master, so please submit all PRs based on themasterbranch. We generally will cherry-pick relevant bug fixes onto the current release branch.What problem is this PR intended to solve?
This allows the C code to compile when compiling with the C compiler from Microsoft Visual Studio (Microsoft (R) C/C++ Optimizing Compiler Version 19.26.28805 for x64).
This is relevant because if you compile Ruby with that compiler, that would be the compiler you use for compiling native extensions.
Related to #2015.
Have you included adequate test coverage?
These changes are not intended to cause any changes in behavior.
Does this change affect the C or the Java implementations?
These changes are only relevant to the C implementation.