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

Upstream/remove blobfish.patch #108

Closed
adam-urbanczyk opened this issue Mar 10, 2024 · 23 comments
Closed

Upstream/remove blobfish.patch #108

adam-urbanczyk opened this issue Mar 10, 2024 · 23 comments
Labels

Comments

@adam-urbanczyk
Copy link
Contributor

adam-urbanczyk commented Mar 10, 2024

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#139

CC: @looooo , @blobfish, @efferre79

@efferre79
Copy link

the patch has been introduced in the @blobfish occt repository here, if that patch is strictly needed then I would strongly suggest to submit it for integratation upstream otherwise is only causing conflicts between upstream opencascade and conda-forge one

@blobfish
Copy link

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.

@efferre79
Copy link

Just to have a complete picture, what is the purpose of that patch? Moreover is IsTangentFaces() overloading used only by the software in your repositories or is there any other project outside?

@blobfish
Copy link

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.

@efferre79
Copy link

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.

@blobfish
Copy link

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.

@efferre79
Copy link

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?)

@blobfish
Copy link

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.

@whophil
Copy link

whophil commented Mar 13, 2024

It is definitely confusing that the occt package on conda-forge contains source patches beyond changes which make it possible to build on conda-forge.

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 occt in freecad?

From the conda packaging perspective, there are a number of ways to resolve this:

  • Separate feedstock for "official" and "patched" variants
  • One multi-output feedstock (variants with and without patches)

@adrianinsaval
Copy link
Contributor

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

@adam-urbanczyk
Copy link
Contributor Author

@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...

@whophil
Copy link

whophil commented Mar 13, 2024

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

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., occt-freecad - assuming these changes are for the FreeCAD project.

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 occt package as well. Having two variants could let developers pick the one their application needs.

@efferre79
Copy link

There are certainly downstream projects that would prefer to have the "true" upstream occt package as well. Having two variants could let developers pick the one their application needs.

I like the idea of variants if the patch should be preserved.

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

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).

@whophil
Copy link

whophil commented Mar 14, 2024

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).

This is the conda-forge OCP feedstock, so I am only taking about conda-forge builds of freecad

@efferre79
Copy link

efferre79 commented Mar 14, 2024

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).

This is the conda-forge OCP feedstock, so I am only taking about conda-forge builds of freecad

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

@blobfish
Copy link

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.

@adam-urbanczyk
Copy link
Contributor Author

OK, your position is clear @blobfish
@looooo what is your take on this, would you consider making this a "clean" occt feedstock?

@adam-urbanczyk
Copy link
Contributor Author

ping @looooo

@adrianinsaval
Copy link
Contributor

#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

@whophil
Copy link

whophil commented May 7, 2024

bumping @looooo again, would be great to get your take on this!

@adrianinsaval
Copy link
Contributor

adrianinsaval commented May 7, 2024

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.

@adam-urbanczyk
Copy link
Contributor Author

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!

@adam-urbanczyk
Copy link
Contributor Author

Solved by #111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants