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

Integrating with my fork #10

Open
rpavlik opened this issue Aug 14, 2013 · 7 comments
Open

Integrating with my fork #10

rpavlik opened this issue Aug 14, 2013 · 7 comments

Comments

@rpavlik
Copy link

rpavlik commented Aug 14, 2013

It looks like you've done some great work here, and I'd like to integrate with your code to come up with a single best fork. There are a few issues I've already reported in trying to use your code, but there are a few questions I have that would be helpful in integration.

  • Any particular reason why the half-baked CMake system from an abandoned upstream branch was used instead of the one I had already made? I made it to be generally useful.
  • The out of order cherry-picking made it hard for me to try to extract your changes (using a "rebase onto"), and it looks like you skipped some patches (intentionally or not). Would you consider cherry-picking your changes on top of a branch based on my latest master? Some of the merge conflicts I'm getting I am not sure how to resolve correctly.
  • Code formatting - This is the bane of branching and merging. What I've done for other projects is come up with an astyle config file and added that, as well as a script to astyle the source code, to the repo. I'm happy to go with your preferred style here to try to make merges easier.

Looking forward to working together!

@Oberon00
Copy link
Owner

I'd like to integrate with your code to come up with a single best fork.

+1

Any particular reason why the half-baked CMake system from an abandoned upstream branch was used instead of the one I had already made? I made it to be generally useful.

The "reason" was that I just needed to use and thus build Luabind, (the included bjam scripts were/are broken) and I had not found your fork by then. The current implementation, however, has nothing to do with the branch I forked then.

The out of order cherry-picking made it hard for me to try to extract your changes (using a "rebase onto")

Sorry. I'm sort of a git newbie.

it looks like you skipped some patches (intentionally or not)

I remember skipping your reimplementation of the numeric builtin converters because it didn't work for me (maybe I damaged it when resolving conflicts or missed another required commit). I also skipped a few commit I had already implemented (e.g. fhoefling's fix for the failing build with gcc (78509cc)) and your usage of BOOST_SYMBOL_IMPORT/_EXPORT, since you seemed to revert it later. However, my fork is still lacking regarding this (I myself use Luabind as a static library).

Code formatting - This is the bane of branching and merging.

Yes. Nearly every cherry pick resulted in a conflict. Only later I learned of the ignore-space-change merge strategy option, but haven't tested it yet.

Would you consider cherry-picking your changes on top of a branch based on my latest master?

Seems like a lot of work, unless ignore-space-change is really all that is needed to resolve (most) conflicts automatically. Also, you now have started to cherry-pick my commits (but I guess I could just skip these).

Are there any features/fixes my fork is missing from yours (apart from the reported issues)?

Also, please be aware that my current implementation of set_package_preload is incompatible with yours (f0211b2).

@rpavlik
Copy link
Author

rpavlik commented Aug 16, 2013

I'm fine with the change to set_package_preload, that is not a problem.

I've worked on merging our branches, after I destroyed a perfectly-good rebased git repo with a recursive call to sed. I now think I have the union of the useful parts from each of our branches. (Mine included additional cleanups and header tweaks, like trying to make object.hpp not such a behemoth and reducing include clutter/cycles, as well as more featureful and clean build system.)

See my branch https://github.com/rpavlik/luabind/tree/merged which started with a mass-cleaned (whitespace cleanup at each commit) version of your master branch merged into my master branch, and which has been improved with additional features in the build taken from your code (all the warnings, etc). It does build with GCC on Ubuntu 12.04 as long as LUABIND_USE_CXX11 is selected, with any of the configuration options selected. (I've added a configured header for the defines that change the interface of the library, which both simplifies things and makes it harder to use wrong). However, it fails regardless of LUABIND_USE_CXX11 with the Clang PPA, as well as with that option disabled for GCC.

Is this a platform you have access to in order to look at the build errors, or should I be sharing those errors in issues?

GCC spits a lot of warnings in the build - would it be worthwhile for me to open a issue and paste them?

@rpavlik
Copy link
Author

rpavlik commented Aug 16, 2013

(oh, and whoah, there's an ignore-space-change merge strategy? My mind is blown.)

@Oberon00
Copy link
Owner

I guess your C++11 errors are caused by your older Boost version: see #7, #11 and commit 296a357 where I added checks for the predecessor macros of the BOOST_NO_CXX11_* family (if you have already included that commit, then please open an issue).

GCC spits a lot of warnings in the build

Strange. I try to always test with gcc 4.8 but have not seen any warnings with my warning flags.

would it be worthwhile for me to open a issue and paste them?

Yes, I would be interested to see them (but just in case, please first apply 296a357).

However, it fails regardless of LUABIND_USE_CXX11 with the Clang PPA

I've currently booted Windows, so I can't check my Clang version right now, but it's even if it's a different (newer) version, it's still strange that it fails there when it works with my version. Maybe the problem is the combination of the standard library (gcc's one is used by default) and Boost version. If the errors persist after the mentioned patch, it might be worth opening an issue.

@rpavlik
Copy link
Author

rpavlik commented Aug 16, 2013

Hmm, so I do include your latest commits in the "merged" branch. I'll open a number of issues here to keep in contact with you (even though the source is in my repo).

@rpavlik rpavlik mentioned this issue Aug 19, 2013
@Bertram25
Copy link

I bet this one is no more on the plate?

@rpavlik
Copy link
Author

rpavlik commented Dec 28, 2021

well, I haven't used Luabind in quite a few years by 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

No branches or pull requests

3 participants