-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Upstream/remove blobfish.patch #108
Comments
Yes, having the changes merged upstream would be ideal. However that would require me signing the OpenCasCade CLA and I refuse to do that, so here we are. You guys should stick to using the official branch or my branch and not a hybrid. FYI: my 7.8.0 branch will be pushed soon as I was waiting for them to publish the updated data for the test suite. |
Just to have a complete picture, what is the purpose of that patch? Moreover is |
I think commit message answers these questions doesn't it? purpose is: Allow blending of faces less than 5 degrees IsTangentFaces() overloading used only by the software in your repositories: No, I added the overload only for opencascade itself. |
As this feedstock aims to provide upstream OCCT and the patch doesn't solve any open bug (e.g. a backport from a newer version), I would vote for removing it. This to avoid unexpected problems downstream. |
The commit in question builds on linux to this day and last year built on multiple versions of mac, windows and linux. I have no idea what you guys have going on here or why a minor change, like adding an overload, would cause you grief. To be 'frank', I don't care enough to investigate. I just made the changes and made them public, I didn't bring the changes to this project. Yes, you should remove any/all my commits. Then stick to the official opencascade branch, as you will not want to reconcile my changes vs the official branch. |
none is blaming you for the patch. I was just trying to investigate why it was included her to understand if there is any really good reason to keep it vs the effort needed later to maintain it as a diverging source code vs upstream. Of course if there was a chance to have it integrated upstream the scenario would be different but you are the first who doesn't care about. Another solution would be to maintain a separate occt feedstock from your fork and keep this one aligned to upstream as you have already suggested previously. Anyway I agree with you that it would be better to have the full coherent package and not an hybrid. BTW The small change is actually making impossible to build occt python bindings when not using conda occt release, for sure the python bindings generator can be improved but this is the status now (it might be patched that too but why to maintain a patch due to another patch?) |
Sorry I didn't mean to get confrontational. Working with OpenCasCade is a huge source of frustration. I think we are on the same page now. Ok. I have little need/interest in python and language bindings. For people with those needs/interests, they should avoid anything of mine. |
It is definitely confusing that the Perhaps @looooo or @adrianinsaval can provide some background about why "blobfish" patches (and others) were included in this feedstock? I assume it is related to usage of From the conda packaging perspective, there are a number of ways to resolve this:
|
there are several bugfixes relevant to freecad on the blobfish patches, but it might be worth looking at each individually and cherry pick only those that are important |
@blobfish would you accept some EUR donation to actually upstream your patch or transfer the copyright to someone who would do that? I think it would benefit the whole community... |
Thanks @adrianinsaval. I wonder if this feedstock provides a version of OpenCascade that is sufficiently different from the upstream that it should be called something else - e.g., I wonder about the licensing and attribution of the patches. I'm not an expert in that area, though. There are certainly downstream projects that would prefer to have the "true" upstream |
I like the idea of variants if the patch should be preserved.
The patch is currently a single file merging different commits from blobfish fork and there is no immediate evidence of which problems are solved (from the user point of view). In case you decide to maintain the patch or a subset of it, I would suggest to divide it in separate files, one for each commit or topic (in case there is a series of commits on a specific issue) and to document the problem solved (e.g. a link to the freecad forum, bugs, source commits, etc). The documentation helps with the "consumers" and provides the proper credits to the original author. BTW if you look in freecad sources, the building system uses opencascade present in the system which typically is not installed by conda (at least on Linux). So there is no way to bind freecad with a specific patched version of opencascade. By just assuming that freecad would be built against this package it is not a good practice in my opinion (different freecad users would get nonidentical experience). |
This is the conda-forge OCP feedstock, so I am only taking about conda-forge builds of |
I can understand that is used for freecad but this doesn't mean it must be used only for freecad. Recalling the manifest, conda-forge is a community led collection of recipes, build infrastructure and distributions for the conda package manager. I think that here we are all volunteers and I personally try to improve things so that the whole community of open source users would benefit. Fragmentation is not good. For sure it would be easier to track changes and make things clear by implementing two variants of this feedstock (e.g. "vanilla" and "freecad" flavours). Freecad conda builds can then point to the proper variant. This is your suggestion above and I like it, I don't have experience on how to write a feedstock or what is possible to do so for sure the maintainers know better. EDIT: of course the creation of many variants implies fragmentation, the best solution is to push everything upstream as much as possible |
Sorry, I know my reluctance to sign the OpenCasCade CLA is causing people grief. If I thought it was possible to play nice with corporations, I would still be writing c++ plugins for a high tier CAD system and making a nice living instead of living in poverty. If OpenCasCade wants to start a non-profit foundation, I would seriously consider signing a CLA with that entity, but never a for profit, private company. I have never made an official fork. I have a branch of 20 commits(at this time) that I rebase upon new OpenCasCade releases. Packaging probably shouldn't consider anything other than an official fork. At this point, I suggest leaving my branch for people willing to compile it. A lot of 'FreeCAD' flowing around this thread. I did some work on freecad years ago and still participate in the forum, but I am not involved in the project anymore. So if you guys go against my recommendations and create a separate package, I ask you to not include 'FreeCAD' in the name. If you want a cad system in the name use occt-cadseer or occt-blobfish as there maybe some recognition to my screen name. |
ping @looooo |
#111 I'll start by separating each patch and remove only the problematic one mentioned here. I think the rest of the patches are fine |
bumping @looooo again, would be great to get your take on this! |
I merged the PR as a first step before too many packages get rebuilt against occt 7.8.1. I'm leaning towards leaving the remaining patches in but I'm open to suggestions. |
Works for me, thanks! |
Solved by #111 |
Comment:
Could the
blobfish.patch
be upstreamed to OCCT or removed? It is causing some subtle issues when it comes to packaging and mixing conda-forge and non-conda-forge packages. See CadQuery/OCP#139CC: @looooo , @blobfish, @efferre79
The text was updated successfully, but these errors were encountered: