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

Add builder for CGAL julia wrapper #405

Closed
wants to merge 0 commits into from
Closed

Conversation

rgcv
Copy link
Contributor

@rgcv rgcv commented Jan 17, 2020

Submitting a recipe for building the wrapper code for a (very rough WIP) package that exposes some of CGAL's types and functionality: CGAL.jl.

A few notes:

  • Akin to the libcxxwrap-julia builder recipe (see Add builder for libcxxwrap-julia #357), I too am downloading julia during the build process as a temporary workaround.
  • Unlike the aforementioned library, I was met with linking errors when linking against its cxx03 string abi binaries. I suspected I might encounter this error (JlCxx: Expand C++ string ABI + install LICENSE.md #393 (comment)). Filtering them out appeared to work.
  • Since, apparently, my build system isn't a simple build system anymore, dlls in mingw builds are being manually copied to bin as well. Any pointers on removing this from the build script would be much appreciated.

Any and all feedback will be more than welcome!

@rgcv rgcv force-pushed the master branch 2 times, most recently from 22ef3cd to c7c1457 Compare January 17, 2020 21:30
@giordano
Copy link
Member

Unlike the aforementioned library, I was met with linking errors when linking against its cxx03 string abi binaries. I suspected I might encounter this error (#393 (comment)). Filtering them out appeared to work.

What error did you get exactly? Are you sure you need to expand here?

Note that there is no license in the generated tarball

@rgcv
Copy link
Contributor Author

rgcv commented Jan 19, 2020

What error did you get exactly? Are you sure you need to expand here?

I was getting a linking error with a name-mangled reference to what I can only assume is a c++11 basic_string.. I think I can still find the exact logs, I'll get back to you on that.

Note that there is no license in the generated tarball

Misfire on my behalf, I'll address it ASAP

@giordano
Copy link
Member

giordano commented Jan 19, 2020

I was getting a linking error with a name-mangled reference to what I can only assume is a c++11 basic_string.. I think I can still find the exact logs, I'll get back to you on that.

If so, this issue has been likely fixed by JuliaPackaging/BinaryBuilder.jl#604, but if cgal has symbols involving basic_string it's also very much probable that you need to expand cxxstring ABIs for that package first. Can you please run again locally the builder for cgal with the latest master of BinaryBuilder.jl and see if you get a warning about the C++ string ABI? You can do it for any platform, don't need to run again for all of them

@rgcv
Copy link
Contributor Author

rgcv commented Jan 19, 2020

If so, this issue has been likely fixed by JuliaPackaging/BinaryBuilder.jl#604, but if cgal has symbols involving basic_string it's also very much probable that you need to expand cxxstring ABIs for that package first.

I'm scratching my head now, remembering a relatively important detail: since 5.0, CGAL's minimum supported version of the C++ standard is C++ 11, so I'm guessing I'll have to expand the ABI versions and only include cxx11? I'm not too sure, but it makes sense in my head, at least.

Can you please run again locally the builder for cgal with the latest master of BinaryBuilder.jl and see if you get a warning about the C++ string ABI? You can do it for any platform, don't need to run again for all of them

Will get on that as well!

@giordano
Copy link
Member

I'm scratching my head now, remembering a relatively important detail: since 5.0, CGAL's minimum supported version of the C++ standard is C++ 11, so I'm guessing I'll have to expand the ABI versions and only include cxx11? I'm not too sure, but it makes sense in my head, at least.

The C++ string ABI of GCC doesn't have to do with the C++ standard used, this is exactly the source of the incompatibility mentioned in RootFS.md: when using GCC, you can compile a C++11 program with both C++03- and C++11-style ABIs. For more information, see also the the Dual ABI page in the GNU C++ Library manual.

@rgcv
Copy link
Contributor Author

rgcv commented Jan 19, 2020

The C++ string ABI of GCC doesn't have to do with the C++ standard used, this is exactly the source of the incompatibility mentioned in RootFS.md: when using GCC, you can compile a C++11 program with both C++03- and C++11-style ABIs. For more information, see also the the Dual ABI page in the GNU C++ Library manual.

Thank you for that! I'm still relatively new to some of these intricacies, still can't quite wrap my mind around it. But it makes sense, yes, of course.

On another note, I indeed do get some warnings about using std::string values. I'll submit a PR to expand the C++ string ABI version in the CGAL builder.

@rgcv
Copy link
Contributor Author

rgcv commented Jan 19, 2020

..I did not mean to close the PR, probably shouldn't just have force-pushed, deleting the commit.. My sincerest apologies for the mess.

@giordano
Copy link
Member

giordano commented Jan 19, 2020

No worries, but keep in mind that working in a branch that has the same name as the upstream branch is a very bad idea, also because this workflow doesn't let you submit multiple pull requests at the same time, which I think is what happened here. I'm pretty sure that GitHub gives you also prominent warning if you try to do so, pointing to https://blog.jasonmeridth.com/posts/do-not-issue-pull-requests-from-your-master-branch/. You should always create a new branch for each PR

@rgcv
Copy link
Contributor Author

rgcv commented Jan 19, 2020

No worries, but keep in mind that working in a branch that has the same name as the upstream branch is a very bad idea, also because this workflow doesn't let you submit multiple pull requests at the same time, which I think is what happened here. I'm pretty sure that GitHub gives you also prominent warning if you try to do so. You should always create a new branch for each PR

Roger that. Been too accustomed work on a single branch recently. Lesson learned, that's for sure! Will be submitting a new PR properly now 😉

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.

2 participants