-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
ifcopenshell: 240611 -> 0.7.10, fix build and activate most tests #312381
base: master
Are you sure you want to change the base?
Conversation
2ae653e
to
94650c5
Compare
Ok now it's compiling, but I have a weird problem with the runtime lib. libxml2 is missing at runtime:
I have no idea how to fix this problem, for me |
94650c5
to
350b351
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/fixing-ifcopenshell-libxml2-not-found-by-linker-at-runtime/45764/1 |
Ok so I fixed my libxml2 issue by making this a regular package and not a python package (see last temp commit). So it's something to do with the way python packages are made. Can I - and should I - split the lib and the python packaging into 2 derivations? That would allow freecad to depend only on one of them for instance. |
That being said, my end goal is freecad ifc support, and apparently this needs python bindings, so I might not personnally need the split. |
I feel that I need mkDerivation (because it executes the patchElf phase) and buildPythonPackage. How can I combine the 2? |
fa560fb
to
192e18c
Compare
192e18c
to
689d08e
Compare
689d08e
to
f0b8a76
Compare
f0b8a76
to
af35a90
Compare
af35a90
to
67d241e
Compare
67d241e
to
640e0fc
Compare
Result of 2 packages failed to build:
|
918295b
to
aaba05e
Compare
Result of |
pkgs/top-level/all-packages.nix
Outdated
@@ -8949,8 +8949,6 @@ with pkgs; | |||
|
|||
idle3tools = callPackage ../tools/system/idle3tools { }; | |||
|
|||
ifcopenshell = with python3Packages; toPythonApplication ifcopenshell; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that this is useful if python provides some cli tools. This is not the case here.
There are some binaries in $out/bin
though. I believe that to make things properly, these should be in another derivation in libraries/, with python build disabled. Or maybe this just uses the current default python3 version and in this case I should keep this line?
@@ -5816,7 +5816,9 @@ self: super: with self; { | |||
|
|||
ifconfig-parser = callPackage ../development/python-modules/ifconfig-parser { }; | |||
|
|||
ifcopenshell = callPackage ../development/python-modules/ifcopenshell { }; | |||
ifcopenshell = callPackage ../development/python-modules/ifcopenshell { | |||
inherit (pkgs) libxml2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an error about libxml2 missing if I don't add this line, no ideas why.
I believe this is now ready. I still have some questions though :-) The main one is whether or not I should keep it in "all-packages.nix"
For some reason, this script says "nothing to build". As the build is broken since ages, I believe we don't really need to check dependent packages ;-) |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/fixing-ifcopenshell-libxml2-not-found-by-linker-at-runtime/45764/3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this. I know building IfcOpenShell is not easy.
nixpkgs-review pr 312381
also displayed "nothing to build for me" but dropped me inside a shell where I could build the derivation with nix build ./nixpkgs#python3.pkgs.ifcopenshell
.
The executables (IfcConvert
, IfcGeomServer
and svgfill
) seem to work or at least don't crash unexpectedly when provided no real input. I no longer work with IFC so I have no way to test with real data. This is also way I would like to step down as maintainer. Do you want to take over, @autra?
I'm a bit confused about the versioning. The fetched code seems to be from v0.7.10 but you reference a v0.8.0 Makefile in the comments (which is probably OK as it builds and the executables seem to run). The built IfcConvert
also reports itself as 0.8.0
. What is going on here?
Otherwise lgtm
indeed 😅
I can take over indeed, as I will be working with ifc files at least for a year (maybe longer).
Honestly... me too. Their release process looks very odd to me. It seems that they are constructing a wheel by downloading the previous release build from s3 and copying the so and wrapper.py from there. This sounds like a very very bad practice security-wise (who is uploading to this s3 bucket?), and it's really asking for a xy-type of security issue if you ask me! So my understanding is that they "propagate" the .so (and wrapper.py) from the latest v0.x.0 tag to each intermediate python release. If we want to mimic this behaviour, we should create a native ifcopenshell derivation built from this tag, then copy the .so from this to each python3xxPackages.ifcopenshell. The problem is that we then have a python version mismatch problem that I didn't manage to solve because the native part still needs python to build its .so and wrapper.py. I guess we should suggest something saner upstream, but I'm not even sure what. For now, I've decided to take everything from the python tag, but I can try again the split between native package and python packages if you have a solution to my python version mismatch issue. What do you think about all that? |
This issue : IfcOpenShell/IfcOpenShell#3927 actually talks about that. |
aaba05e
to
b4001e6
Compare
I've started to see what I can contribute upstream to make things easier. In the meantime, I'd say we can merge this and iterate from this. @fehnomenal I've added a commit that changes the maintainer. |
By relevant release, I mean the ifcopenshell-python release, and not the blenderbim release
b4001e6
to
3da10a1
Compare
I fixed a nixf-tidy error and added opencollada as buildInputs. |
Thanks for taking the time to investigate all of this and also thanks for replacing me 👍🚀 |
Description of changes
Fix the ifcopenshell build.
Fix #312380
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.