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 the example for MSVC in the Embedding Julia documentation #44587

Closed

Conversation

Taaitaaiger
Copy link
Contributor

The current example for embedding Julia with MSVC fails to compile, this pull request provides a working example using code from julia_headers.hpp from CxxWrap

@ViralBShah ViralBShah added embedding Embedding Julia using the C API docs This change adds or pertains to documentation labels Mar 12, 2022
@ViralBShah
Copy link
Member

This feels a bit fussy. Does this mean we need to something within Julia to make it nicer?

@Taaitaaiger
Copy link
Contributor Author

Do you mean nicer in the sense that the additional includes and using C++ wouldn't be necessary? That would definitely be better IMO, but I'm not sure if that's possible because Julia uses features like _Atomic that MSVC doesn't support.

@ViralBShah
Copy link
Member

Ok. I suppose having a working example is better in any case. @vtjnash can you review?

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to make no sense to me. I think it is all old support content for headers that we deleted in v1.8?

@Taaitaaiger
Copy link
Contributor Author

The issue is quite old, actually.

The example that's currently available already failed to build with MSVC using Julia 1.5: #37957
Users of libcxxwrap have run into this problem: #34201
Eventually @toivoh provided a working solution here: #39595

I decided to provide a new example based on what CxxWrap does because that example works.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 6, 2022

Right, but you are targeting v1.9 with this PR, not old versions like v1.5.x

@Taaitaaiger
Copy link
Contributor Author

This PR doesn't affect any code but only changes some documentation. I think you might be confused with this recent PR I've made recently: #44842

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 6, 2022

No, I mean this documentation change seems to specify incorrect changes, needed only to work around bugs in old versions of Julia

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 13, 2022

Closing as I believe this does not apply to master, only old versions. Please let me know if that is wrong and we will try to fix master not to need this.

@vtjnash vtjnash closed this Apr 13, 2022
@barche
Copy link
Contributor

barche commented Apr 13, 2022

I was just checking this, it's true that the original "No Target Architecture" error appears only in Julia 1.6 and 1.7, but using the latest nightly there still is an error that is fixed by adding these lines. The error I get when removing the workaround from CxxWrap is:

C:\hostedtoolcache\windows\julia\nightly\x64\include\julia\ios.h(113,33): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [D:\a\libcxxwrap-julia\libcxxwrap-julia\build\cxxwrap_julia.vcxproj]
C:\hostedtoolcache\windows\julia\nightly\x64\include\julia\ios.h(113,22): error C2146: syntax error: missing ';' before identifier 'ios_fillbuf' [D:\a\libcxxwrap-julia\libcxxwrap-julia\build\cxxwrap_julia.vcxproj]

With the workaround, it compiles against nightly. Presumably 1.8 has the same issue, but there MSVC doesn't work because of #44842.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 13, 2022

ah, I suppose we could add this code (from uv/win.h) to src/support/dtypes.h to make sure that definition is available in standalone mode

26  #if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED)
  1 typedef intptr_t ssize_t;
  2 # define SSIZE_MAX INTPTR_MAX
  3 # define _SSIZE_T_
  4 # define _SSIZE_T_DEFINED
  5 #endif

@barche barche mentioned this pull request Apr 14, 2022
@Taaitaaiger
Copy link
Contributor Author

Thanks for checking, @barche, and providing a better PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation embedding Embedding Julia using the C API system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants