Skip to content

Conversation

@vovkasm
Copy link
Contributor

@vovkasm vovkasm commented Feb 16, 2013

This patch adds C++ support for EUMM. It introduse parameter (and Makefile macro) with name XSTARGET_EXT - it is suffix for files prodused from .xs (.c by default).

This allows modules written in C++ does not use various hacks in order to run the C++ compiler for ".с" files.

I personally test this change with:

  1. linux
  2. strawberry perl 64 on Win7

Only warning. I blindly made changes in ExtUtils::MM_VMS (because I personally can't test for this platform)

Add option XSTARGET_EXT to WriteMakefile (it is ".c" by default).
Generated Makefile under ExtUtils::MM_Unix now contains variable
XSTARGET_EXT and uses it as suffix in files produced by xsubpp.
Also add right files in C_FILES list.
Plus try fix all other xs_c,xs_o methods (only in VMS really).
This commit skips t/xscpp.t tests if user has no C++ compiler.
@schwern
Copy link
Contributor

schwern commented Mar 15, 2013

Impressive work, and tests!

To move this forward it needs two things. First is documentation. The MakeMaker XS documentation is a mess, if it doesn't slot in there maybe add an entry to ExtUtils::MakeMaker::FAQ about how to incorporate C++ code into the project.

Second is having somebody who understands XS and C++ look this over. Your changes look small and don't rewrite large swatches of code, which is good from my perspective, which is lacking understanding of XS and C++. Lemme see if I can scare up a reviewer.

@preaction
Copy link

It passes my average C++ knowledge, and I'm going to try it against the two XS C++ projects I have right now.

@preaction
Copy link

I get a WARNING: "XSTARGET_EXT" is not a known parameter, but the resulting Makefile does include the XSTARGET_EXT variable. And then "cc" correctly compiles the .cpp file as C++ (which means I can remove my "CC => 'g++'" option in WriteMakefile()).

So, it works for me.

A warning: If you're going to start using this in an existing project, be sure to make clean before you add XSTARGET_EXT. If you don't remove the old .c files, it will try to compile them (which will probably fail). I spent 10 minutes thinking this didn't work because those old .c files were hanging around.

@schwern
Copy link
Contributor

schwern commented Mar 16, 2013

@preaction Thanks for having a look. I think I found where the warning is coming from.

Good note about cleaning up the old .c files. That should go into the FAQ about this when it gets written.

@vovkasm
Copy link
Contributor Author

vovkasm commented Mar 16, 2013

Thanks for review!

I'am just fix warning as suggested + fix formatting...

I will try write small intro, but almost certainly it will need help to tune wording, grammar etc... (because my english is bad ;-)

…so improve formatting for XSTARGET_EXT option description
@vovkasm
Copy link
Contributor Author

vovkasm commented Mar 17, 2013

I'am just add some docs about XSTARGET_EXT and using it to build XS with c++ compiler. I will be very grateful for pointing me in the syntactic and stylistic errors.

@vovkasm
Copy link
Contributor Author

vovkasm commented Apr 6, 2013

So... can I do something to help merge this request now?

Because after that, I can continue work to improve C++ support:

  1. Teach xsubpp to support report correct filename in "line" directive
  2. Patch MakeMaker to use that functional
  3. Teach xsubpp generate the code for correct processing of C++ exceptions

@Leont
Copy link
Member

Leont commented Apr 7, 2013

I think it's a useful hint for compilers that use extensions for heuristics, but there will be compilers that don't support that. That portability limitation should be explained.

Secondly, it seems a «make test» invocation is missing in t/xscpp.t (which is actually a bug it inherited from t/xs.t). When fixing that, it fails just as I had expected: Thing is, it doesn't solves the issue of linking C++. You can't generally expect a C compiler to link a C++ object correctly.

@vovkasm
Copy link
Contributor Author

vovkasm commented Apr 7, 2013

Firstly, thanks for review!
I will try to investigate all this and update patch!

vovkasm added 4 commits April 8, 2013 02:20
Use quiet mode of the ExtUtils::CBuilder instead of
magic with STDOUT/STDERR
Remove preprocessor tests for __cplusplus, we already know that this is
C++
1. Make xs.t and xscpp.t really run "make test"
2. Fix XSCPP module Makefile.PL to add libstdc++ to link
@karenetheridge
Copy link
Member

Has this work stalled? it looks like it was almost complete.

@vovkasm
Copy link
Contributor Author

vovkasm commented Sep 21, 2013

Yes, in a sence.
The main problem is that test code not really crossplatform/crossenvironmental.
I see two possible solutions:

  1. Do not pretend to support all platforms for this time and rewrite test to only check file names but not compilation.
    • Pros. I can do this.
    • Pros. Faster
    • Cons. Not immediate help users to write XS on C++ (but step towards to)
  2. Try support compilation of test (may be disable it for some environments).
    • Pros. Complete solution
    • Cons. Very hard work

So what solution acceptable by community?

@mohawk2
Copy link
Member

mohawk2 commented Aug 4, 2014

Surely the right way is:

  • check files are created right
  • carefully check that a suitable compiler and lib are available and run make test, otherwise skip.

@mohawk2
Copy link
Member

mohawk2 commented Aug 4, 2014

@vovkasm, feel like the tiny tweak this will take? Or want a hand?

@mohawk2
Copy link
Member

mohawk2 commented Aug 4, 2014

By the way, if you did decide to go for your approach 2, this already does detection of compilers etc, so ought to make life easy: https://github.com/daoswald/Inline-CPP/blob/master/Makefile.PL

Indeed, if EUMM gains the capability to do this itself, it will simplify I::CPP's Makefile.PL. Including @daoswald in case he's interested.

@daoswald
Copy link

daoswald commented Aug 4, 2014

I am interested. In addition to Inline::CPP's (ugly but necessarily so) Makefile.PL, you can also look at ExtUtils::CppGuess for work in the area of proper detection of compiler name and libraries for linking purposes.

In fact, there's a test included in Inline::CPP's suite that provides a comparison of my Makefile.PL selection versus the ExtUtils::CppGuess selection so that when I look at test FAILs I can see if one might do a better job than the other. There is still much work to do in this area; we still seem to not be 100%.

smueller might have additional input (I believe he authored ExtUtils::CppGuess).

@mohawk2
Copy link
Member

mohawk2 commented Aug 5, 2014

Is it crazy to think that we can unify I::CPP's Makefile.PL, @tsee's https://github.com/tsee/extutils-cppguess, and this, and just use (probably) E::CppG?

* commit '3f38c3a0136464f543c714a10d3fba557afc0d2e':
  Only bundle JSON::PP::Compat5006 if $] < 5.008
@rehsack
Copy link
Contributor

rehsack commented Aug 22, 2014

Am 20.08.2014 um 21:47 schrieb Michael G. Schwern [email protected]:

On 8/20/14 11:46 AM, Jens Rehsack wrote:

@schwern https://github.com/schwern writing some code using STL is
from c++ point of view trivial (CXXLD=$(CXX) and you're mostly done).
Tricky is always extra stuff - Wx is a really non-trivial case.

Show me, don't tell me. I want to see a full, working, portable
implementation. If it's trivial, it should be no problem for you. I
think we will learn a lot.

You can study some c++ code by reviewing my c++ projects on GitHub.
Seriously, I don't have the time to show you a constructed example,
and I don't have the tuits to fight the EU::MM integration afterwards.

When you (or someone else) has serious interest - I can develop the
c++ part of a smaller project (I do not see a reason for just another
json parser). I'm also willing to explain on errors how to go further.

I'm not a C++ programmer, I can't do it myself. I suggested wrapping a
JSON parser. Make it happen. This is the last time I will ask for one,
and I withhold my support for merging this until I see it.

Ask for something more valuable.

As to using non-core modules, now that configure_requires is solidly in
the toolchain, I have no problem with C++ authors having to add a CPAN
module as a configure_requires dependency. I don't feel we need to
bundle it, it's not universally needed. "make dist" can even be made
smart enough to detect that a CPAN dependency is in use and add it to
configure_requires in the META file as Module::Build will sometimes do.

I am much, much less enamored of depending on non-CPAN programs like
autoconf and the like, even with an Alien CPAN wrapper. They're hard to
bundle.

There is a Config::AutoConf already - no need to use native (non-portable)
one!

They're hard to upgrade from a CPAN module. They don't work on
Windows. I'm not sure if that's being discussed here, but I register my
displeasure.

I'm going to be totally offline for about two to three weeks. I hope to
see a working, portable sample C++ distribution implementation when I
get back!

Not from me, however. Please offer a bit more than merging in case of
seeing a working example. You can either pick one of my modules which
might benefit from an XS part and is OO enough or not widely used to
allow using C++ (LMU or Hash::Merge are small but can't do because of
to wide used and I would kill to much users).

What I can offer would be 2 XS implementations for Hash::Merge - a C
one and a C++ one, with an C::AC detector of what XS/XSPP shall be used.

But the EU::MM side must be done by someone else.

Cheers

Jens Rehsack
[email protected]

@rehsack
Copy link
Contributor

rehsack commented Aug 22, 2014

I have a much better module proposal for testing: Lib::Log4cplus (perl-wrap Logging Framework for C++) and link using c++ in Unix::Statgrab when log4cplus is used.

Anyway - I would need help in doing the EU::MM integration - the right flags and cxx switches will be delivered by Config::AutoConf.

@mohawk2
Copy link
Member

mohawk2 commented Aug 22, 2014

The vital question is: which EUMM params would you need to pass, that you can't currently? I predict XSTARGET_EXT would be necessary, any others?

@rehsack
Copy link
Contributor

rehsack commented Aug 23, 2014

Am 22.08.2014 um 21:35 schrieb mohawk2 [email protected]:

The vital question is: which EUMM params would you need to pass, that you can't currently? I predict XSTARGET_EXT would be necessary, any others?

I have no clue what XSTARGET_EXT is for and I cannot guess it.

I need to pass generally
CPP, CPPFLAGS, LD, LDFLAGS, AR, ARFLAGS, RANLIB, RANLIBFLAGS, ... (see that eg. in autoconf manual)

  • for C:
    CC, CFLAGS, XSC, XSCFLAGS, CCLD, CCLDFLAGS
  • for C++
    CXX, CXXFLAGS, XSXX, XSXXFLAGS, CXXLD, CXXLDFLAGS
  • for f77/f95 - all newer fortran compilers are typically covered by f95
    F77, F95, F77FLAGS, F95FLAGS, ...
  • Cobol, Ada, Erlang, Java (gcj) ... similar

There should be one difference kept in mind - writing an XS module using C++ but native (C-)API can do using xsubpp (XSC) but needs CXX to compile and CXXLD to link while C++ XS with c++ types (STL ones?) one need xspp (XSXX) and similar for f95, cob ...

The linker to be used is usually:

ccld < cobld < fnnld < cxxld

C++ is the most complex environment, a suitable objdump utility is required when mixing more than 2 languages (to get the required runtime libs).

Best regards

Jens Rehsack
[email protected]

@rehsack
Copy link
Contributor

rehsack commented Aug 30, 2014

No reply within a week but actively discussion on other topics I rate this topic evolves not Perlish enough?

@mohawk2
Copy link
Member

mohawk2 commented Aug 30, 2014

@rehsack, maybe you didn't mean it that way, but that looked like you sniping. I'd prefer that instead you be more proactive in answering questions put to you on various topics, such as #75 (comment).

@vovkasm, rather than merge across other more recent changes, which will cause big problems to merge this PR, could you back out that merge, then rebase your master against current PTG master and then push -f it? If you need help with the fairly hairy git commands involved, I can assist.

@vovkasm
Copy link
Contributor Author

vovkasm commented Aug 30, 2014

@mohawk2, okay I will do that way, but first I need to complete @schwern proposal with jsoncpp ;-)

@mohawk2
Copy link
Member

mohawk2 commented Aug 30, 2014

@vovkasm, I strongly suggest you bring your copy of EUMM up to date first, so you're not aiming at a moving target. If you're happy to take this forward by yourself, please do. If I can help (possibly by seeking wisdom from @daoswald), let me know.

@rehsack, by the way - as part of my "perl in place-with-space" magnum opus, I'm currently fixing up dmake on https://github.com/mohawk2/dmake - non-Perlish enough for you? ;-)

@mohawk2
Copy link
Member

mohawk2 commented Sep 3, 2014

From @daoswald:

http://cpansearch.perl.org/src/DAVIDO/Math-Prime-FastSieve-0.19/lib/Math/Prime/FastSieve.pm
Look at the END block. This is a module that keeps an Inline dependency. There's a small hurdle that one has to jump through to avoid m42 warnings from Inline if the module is 'required' rather than 'used'. It's possible that there's no better solution, but I'm wondering if you can think of anything Inline can be doing differently in this respect.

The module above is on CPAN and uses Inline::CPP. It passes tests on 226 systems, and only has the death penalty on one. @vovkasm, feel like extracting ideas from this? Or are you busy on other stuff?

@Leont
Copy link
Member

Leont commented Sep 4, 2014

The module above is on CPAN and uses Inline::CPP. It passes tests on 226 systems, and only has the death penalty on one. @vovkasm, feel like extracting ideas from this? Or are you busy on other stuff?

That's 226 systems, but only two compilers: gcc and clang. Not even msvc appears to be represented.

@bulk88
Copy link
Contributor

bulk88 commented Sep 5, 2014

I am a Visual C user. I'll ignore the lack of VC compatbility, since it is trivial https://metacpan.org/source/BJOERN/Graph-NewmanGirvan-0.3/Makefile.PL#L32 . I disagree with XSTARGET_EXT. XSTARGET_EXT says a module must be entirely C++ or C, not a mix of both. The better solution would be either .xs.cpp and then a .cpp.obj rule, and use an explicit dependency name (Foo.obj) to trigger the .xs->.cpp->.obj building, or a array ref or hash ref passed to %options should say which xs files are C++, and everything left out is C.

@vovkasm
Copy link
Contributor Author

vovkasm commented Sep 5, 2014

@bulk88, hmm... interesting

XSTARGET_EXT says a module must be entirely C++ or C, not a mix of both.

Did you mean that if XSTARGET_EXT is ‘.cpp’ then all *.xs files in dist will be translated to *.cpp?
Yes it is not very good, but XSOPT argument also global in that sense.

And no, if you mean that XSTARGET_EXT says a module must be entirely C++. XSTARGET_EXT simply says what suffix *.xs files should have after translating with xsubpp, you of course may have mix of *.cpp and *.c files in dist and they will be processed as expected. So you may write module in C++ and embed C-only library.

But I don’t against the idea of using ’special’ suffix for XS files to mark they as ‘XS with C++’... In implementation there are some problems (that I can see):

  1. portability (this will need full C++ support)
  2. suffix like ‘.xs.cpp’ - do all systems supported by perl, allows to specify long suffix in file names?
  3. rules to form .xs -> .o translation in EU:MM is not so trivial so we will need to ‘scale’ this hacks to .xs.cpp -> .o translation.

@mohawk2
Copy link
Member

mohawk2 commented Sep 5, 2014

Open the question up to other participants: does .xs mean C? Should there be .xsp for C++? We should be sure we're dealing with the "worst case" - a module with both C++ and C XS.

@rehsack
Copy link
Contributor

rehsack commented Sep 6, 2014

@mohawk2 - a bit of sniping whom?

I just recognized a stalling, but important issue discussion. My point is:

ExtUtills::MakeMaker is the Perl equivalent to AutoMake - both are makefile generators.
While Automake uses extensible templates, EU::MM let's developers hack makefile generation is a not so smooth way (MY:: ...)
Additionally - see libstatgrab/Unix-Statgrab@8d450aa - EU::MM doesn't rely on a environment checker a la autoconf. Call it as you want - but there should be a separate utility (maybe bundled in a stripped down variant) checking for environment (CC, LD, AR, DLLD, CXX...) and EU::MM should take those parameters.

I think, before extensions a la XSTARGET_EXT can be reasonable implemented, kind of refactoring in the EU::MM kernel are necessary to allow building a "used tool" fundament.

When I would do a Lib::Log4CPlus - I would run into similart issues @vovkasm describes, because I would create neat XS and call the C++ API in it - no export and mungle of C++ types to Perl.

@bulk88
Copy link
Contributor

bulk88 commented Sep 6, 2014

Did you mean that if XSTARGET_EXT is ‘.cpp’ then all *.xs files in dist will be translated to *.cpp?

Yes.

Open the question up to other participants: does .xs mean C? Should there be .xsp for C++? We should be sure we're dealing with the "worst case" - a module with both C++ and C XS.

Yes. Suffix rules is how makefiles are supposed to work in spirit. So .xsp is the easiest way. Just make your file named .xsp and EUMM does all the rest.

ExtUtills::MakeMaker is the Perl equivalent to AutoMake - both are makefile generators.
While Automake uses extensible templates, EU::MM let's developers hack makefile generation is a not so smooth way (MY:: ...)
Additionally - see libstatgrab/Unix-Statgrab@8d450aa - EU::MM doesn't rely on a environment checker a la autoconf. Call it as you want - but there should be a separate utility (maybe bundled in a stripped down variant) checking for environment (CC, LD, AR, DLLD, CXX...) and EU::MM should take those parameters.

That sounds like another bug. I am not going to test or IDK the status of cmd line env vars affecting the make driver building process, or affecting the makefile file's macro contents.

@daoswald
Copy link

daoswald commented Sep 6, 2014

There is more precedent for XSpp
On Sep 5, 2014 10:52 AM, "mohawk2" [email protected] wrote:

Open the question up to other participants: does .xs mean C? Should there
be .xsp for C++?


Reply to this email directly or view it on GitHub
#45 (comment)
.

@rehsack
Copy link
Contributor

rehsack commented Sep 6, 2014

XSpp is not a precedent for anything.

@rehsack
Copy link
Contributor

rehsack commented Sep 6, 2014

Am 06.09.2014 um 11:51 schrieb bulk88 [email protected]:

Did you mean that if XSTARGET_EXT is ‘.cpp’ then all *.xs files in dist will be translated to *.cpp?

Yes.

Open the question up to other participants: does .xs mean C? Should there be .xsp for C++? We should be sure we're dealing with the "worst case" - a module with both C++ and C XS.

Yes. Suffix rules is how makefiles are supposed to work in spirit. So .xsp is the easiest way. Just make your file named .xsp and EUMM does all the rest.

ExtUtills::MakeMaker is the Perl equivalent to AutoMake - both are makefile generators.
While Automake uses extensible templates, EU::MM let's developers hack makefile generation is a not so smooth way (MY:: ...)
Additionally - see libstatgrab/Unix-Statgrab@8d450aa - EU::MM doesn't rely on a environment checker a la autoconf. Call it as you want - but there should be a separate utility (maybe bundled in a stripped down variant) checking for environment (CC, LD, AR, DLLD, CXX...) and EU::MM should take those parameters.

That sounds like another bug. I am not going to test or IDK the status of cmd line env vars affecting the make driver building process, or affecting the makefile file's macro contents.

It's not another bug - and it's not a question of cmd line env vars - it's a fundamental concept issue.
You need to sit down and prove the concepts of Makefile generators and not make to get it.

Please work a little bit in cross-compiling environments, pre-building environments for large data centers having lots of platforms and bulk-build to make a difference on environment variables and configure stage scripts analyzing capabilities of target platforms.

Best regards

Jens Rehsack
[email protected]

@schwern
Copy link
Contributor

schwern commented Sep 6, 2014

I request everyone stop posting on this thread for 48 hours.

Take a step back, it will wait.

Enjoy your weekend.

@ribasushi
Copy link

I was brought in as a 3rd party to look at this issue as well. I see the fundamental problem being the following:

The patch does add working support for C++ with a GNU-compatible toolchain in mind (this means g++ and clang++). The same problem is also present with ExtUtils::XSpp, by which this patch seems to be inspired.

The fundamental question to the EUMM maintainers and power-users in this case is - is the worse-is-better approach of let's get it working on the common C++ toolchain, and let others worry about non-orthodox compilers tenable. I posit it is not, and this patch needs to be put on the backburner, while a discussion (in another thread, or even during QA 2015) is carried out on what is the sane way forward.

The positions of the main participants in this ticket are already clear, I would like to solicit further input specifically from @dagolden, @rjbs, @Tux, @andk and @timbunce.

@Tux
Copy link

Tux commented Sep 17, 2014

riba asked me to reply by mail. I has a long mail typed when the power dropped. Content lost.
Here is what I think I wrote down (summarized) on IRC to riba, who wanted me to copy-paste it here as a reference. Well, it's his fault :)

<[Tux]> I love EU::MM for its simplicity towards the developer
<[Tux]> (the road it takes might be difficult, but the net result is a single Makefile that I can edit if it does not seem to work or does not work as intended)
<[Tux]> example: Net::SSLay looks at $OPENSSL_PREFIX, but it adds -L options to the end op ldflags' search path instead of introspection the actual flags and inserting the new path where it should have been done
<[Tux]> this causes a conflict of -I (include path for compilation) and -L (load path for linker) having compiled against e.g. openssl-1.0.1 and linking against 0.9.8
<[Tux]> that is a guarantee for disaster
<[Tux]> BUT
<[Tux]> it is an easy fix in Makefile
<[Tux]> The developers that use default tool chains will never notice and I - as a HP-UX developer have an easy way to fix it
<[Tux]> net problem (developer load) is low
<[Tux]> Module::Build does not produce a Makefile, but a Build
<[Tux]> I do not like that, as I have to remember that this is not a default build process
<[Tux]> I now have to think about build steps instead of having to think about a solution to the reason why my build failed
<[Tux]> that is not good for concentration, and I think it is undervalued
<[Tux]> On the systems where I have to build with C++
<[Tux]> for whatever reason
<[Tux]> I do not have a single fucking idea of what C++ is used.
<[Tux]> It can be the expensive vendor compiler
<[Tux]> (new compiler can only be baught with cc and c++ bundled)
<[Tux]> or it can be g++
<[Tux]> Since GNU gcc-4 requires dependencies (mpfr, gmp, ...) that can only be built with gcc, you might understand why I gave up building gcc on HP-UX and AIX
<[Tux]> I really do not understand how they could make that mistake
<[Tux]> I probably forgot something out of my rant, but you get the point I how

@dagolden
Copy link

I have only skimmed the thread, so I'm just commenting on design philosophy issues that @ribasushi raised.

When dismissing "worse is better", I think it's also important to remember that "the perfect should not be the enemy of the good". If we can deliver useful features for 99% of end-users, I don't mind throwing an "unsupported" error for the remaining 1%.

My minimum standard for inclusion would be that this should support the default compilers for the vast majority of mainstream Unix-like systems (Linux, (Free|Net|Open)BSD, Darwin, Solaris, Cygwin) and the major compilers for MSWin32 (MSVC and gcc). If support is lacking for VMS or Android or other specialized systems, I think that's OK.

@mohawk2
Copy link
Member

mohawk2 commented Nov 29, 2014

With #168 I've just been making support for multiple *.xs files, which is conceptually related to supporting e.g. 1 C XS, and 1 C++ XS file.

I am increasingly convinced the way forward for C++ XS is to have a different file-extension, almost certainly .xsp, and then to "do the right thing". This really needs to outsource the C++ config stuff to another module, such as ExtUtils::CppGuess.

@mohawk2
Copy link
Member

mohawk2 commented Dec 27, 2014

Nice test cases against which I can test, and from whose current build system I can steal ideas:

Modules that do XS++ support:

Other modules that use XSpp: https://metacpan.org/requires/distribution/ExtUtils-XSpp?sort=[[2,1]]
Others that use ExtUtils::CppGuess: https://metacpan.org/requires/distribution/ExtUtils-CppGuess?sort=[[2,1]]

@vovkasm
Copy link
Contributor Author

vovkasm commented Jan 14, 2015

As #195 provides better solution, this pull request can be freely closed.

@vovkasm vovkasm closed this Jan 14, 2015
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.