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

Recommended upstream? #22

Open
nyalldawson opened this issue Jul 29, 2021 · 10 comments
Open

Recommended upstream? #22

nyalldawson opened this issue Jul 29, 2021 · 10 comments

Comments

@nyalldawson
Copy link
Contributor

I'd like to forward some patches from qgis external copy of libdxfrw (eg qgis/QGIS#44428), but I'd like clarification on whether this repo or https://github.com/codelibs/libdxfrw would be the best place to do this.

Ideally these three maintained forks could be unified into one upstream. Is there general interest for this?

@lordofbikes
Copy link
Member

This repo is the successor of https://sourceforge.net/projects/libdxfrw/, which is the origin of libdxfrw.
There were some discussion and efforts about various forks recently, but I have no conclusion yet.
I'm aware of the codelibs fork, but I'm not sure about their changes.
From my last check, couple months ago, I think that this repo is the most recent and still platform independent. There are some other forks, which diverged to run on single OS only.

What I can remember, this is the first attempt to push fork changes back to this upstream. This is more than welcome, many thanks.
I'll try to check your changes soon and see how our repos diverged.

@nyalldawson
Copy link
Contributor Author

I'll try to check your changes soon and see how our repos diverged.

I'm more than happy to do this and submit PRs, if you are indeed seeking for this to be a software-agnostic upstream and not a librecad specific fork of the library... 👍

@ghost
Copy link

ghost commented Jul 31, 2021

JFTR, Here is libdxfrw fork used in SolveSpace:

@phkahler
Copy link

phkahler commented Dec 7, 2021

@lordofbikes I'd like to delete the solvespace fork and just use this repo if possible since the reason (lack of CMAKE) for forking has apparently been resolved. Could you create an actual release at some point (maybe along with the next librecad release) so we can point to a specific version rather than an arbitrary commit?

EDIT: There are also a number (38 commits) of fixes in our repo. Maybe we should bring some of them over here. LibreCAD isn't really merging PRs very fast, so would this be worth my time?

@lordofbikes
Copy link
Member

@phkahler this fork was initially created for LibreCAD_3 only, which uses CMAKE too.
LibreCAD 2 has libdxfrw still bundled yet, but is in sync with current HEAD.

Latetely we decided to make this repo the official successor of the Sourceforge repo, because there are no active contributors for years now. I have permission to the Sourceforge repo, but we want to leave it untouched yet as we have no response from Rallaz, the founder. A link to GitHub is already placed there on the summary page.

There are a bunch of improvements made to libdxfrw, spread over a couple forks, which we already consolidated here and we want to continue this work. Then we need of course a release near-term.

For a release there is still some work to do. Having the repo for LibreCAD only, made changes to the interface easy, without breaking too much. But as an official source we must consider others needs too and be more careful to avoid deal braking changes. This is true for fork maintainers too, we must consider together then, which changes must be implemented in libdxfrw and which are probably too specific and should be kept in the project interface implementation when possible.
I'm open to any suggestions and support.

E.g. the mentioned CVE's, fixes are included in current HEAD only, but the current HEAD has an interface modification which requires to instantiate 2 abstract methods by the implementer. See description of #20 for details.
Means, we must consider how to handle this now and in future. We can add default implementations to libdxfrw or back port the CVE fixes. But iirc, these two methods are part of bug fixes in LibreCAD 2 and therefor I think back porting doesn't make much sense.

There are already contributions from QGIS fork, with more to come and the same attempt to use this repo for QGIS in future.
So you're welcome to participate here too.

Concerning the merge pace you mentioned, this is simply due to the fact that I'm the only active maintainer for LibreCAD currently. I'm near to finally release the long overdue LibreCAD 2.2.0 release, then I can focus back to a libdxfrw release.

@rpavlik
Copy link

rpavlik commented Jan 2, 2022

FWIW, I did rebase the Solvespace changes here: codelibs/libdxfrw#19 and made some additional cleanups here: codelibs/libdxfrw#20

@TFreudi1
Copy link

@rpavlik: You changed some constructors, for example
DRW_LWPolyline(const DRW_LWPolyline &) = delete;
DRW_LWPolyline(DRW_LWPolyline &&) = default;
but with that the project dwg2dxf won't compile.

See code on the end ... arround row 70 in dx_iface.h that won't compile and .. shame on me ... but I don't know how to change to your new constructor. I just refreshed my knowledge about references but maybe I'm to old :-)

virtual void addLWPolyline(const DRW_LWPolyline& data){
currentBlock->ent.push_back(new DRW_LWPolyline(data&));
}

@rpavlik
Copy link

rpavlik commented Feb 11, 2022

yeah, iirc I tried to turn off as many copy constructors as possible in favor of move constructors, because it wasn't entirely clear to me that the memory management was being handled right. Can you point me to the code you tested with so I can see what's going on?

@TFreudi1
Copy link

TFreudi1 commented Feb 11, 2022 via email

@TFreudi1
Copy link

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

5 participants