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

openctm: fix darwin support #351642

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

yzx9
Copy link
Contributor

@yzx9 yzx9 commented Oct 27, 2024

Things done

Fix darwin support with substitute gcc/g++

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

substituteInPlace "lib/Makefile.macosx" "tools/Makefile.macosx" \
--replace-warn "gcc" "${stdenv.cc.targetPrefix}cc"
substituteInPlace "lib/Makefile.macosx" "tools/Makefile.macosx" "tools/tinyxml/Makefile.macosx" \
--replace-warn "g++" "${stdenv.cc.targetPrefix}c++"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this patch will work on Linux with GCC. Any help would be appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, it is gated by isDarwin, so it should be fine I guess. But probably we want to use stdenv.hostPlatform.is{Linux,Darwin}, ref #341407

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should be fine since it is gated. However, I’m uncertain whether we should patch it in Linux when the user switches stdenv to LLVM. If we proceed with that change, will C++ still remain available in GCC?

The script has been updated to stdenv.hostPlatform.is{Linux,Darwin}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to use gccStdenv, since we don't know whether the author write this for a particular reason or not, and whether they will test building with clang on macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you’re right, gccStdenv is what we need. However, I encountered another error when switching stdenv to gccStdenv 😿

Code changed:

@@ -2,3 +2,3 @@                                                                           
   lib,                                                                                   
-  stdenv,                                                                                
+  gccStdenv,                                                                             
   fetchurl,                                                                              
@@ -11,2 +11,5 @@                                                                         
                                                                                          
+let                                                                                      
+  stdenv = gccStdenv;                                                                    
+in                                                                                       
 stdenv.mkDerivation (finalAttrs: {                                                       
@@ -45,9 +48,4 @@ stdenv.mkDerivation (finalAttrs: {                                      
     + lib.optionalString stdenv.hostPlatform.isDarwin ''                                 
-      substituteInPlace "lib/Makefile.macosx" \                                          
-                        "tools/Makefile.macosx" \                                        
-                        "tools/jpeg/makefile.macosx" \                                   
-                        "tools/zlib/Makefile.macosx" \                                   
-        --replace-warn "gcc" "${stdenv.cc.targetPrefix}cc"                               
-      substituteInPlace "lib/Makefile.macosx" "tools/Makefile.macosx" "tools/tinyxml/Make
-        --replace-warn "g++" "${stdenv.cc.targetPrefix}c++"                              
+      substituteInPlace "tools/tinyxml/Makefile.macosx" \                                
+        --replace-warn "-Wno-format" "-Wno-format -Wno-format-security"                  
     '';                                                                                  

Result:

# some success messages ignored

cd jpeg && make -f makefile.macosx libjpeg.a
make[2]: Entering directory '/private/tmp/nix-build-openctm-1.0.3.drv-0/OpenCTM-1.0.3/tools/jpeg'
cp -f jconfig.macosx jconfig.h
gcc -O2 -Wall -I. -c jcapimin.c
gcc -O2 -Wall -I. -c jcapistd.c
gcc -O2 -Wall -I. -c jcarith.c
gcc -O2 -Wall -I. -c jctrans.c
g++ -x objective-c++ -c -O3 -W -Wall -o sysdialog_mac.o sysdialog_mac.mm
gcc -O2 -Wall -I. -c jcparam.c
gcc -O2 -Wall -I. -c jdatadst.c
gcc -O2 -Wall -I. -c jcinit.c
gcc -O2 -Wall -I. -c jcmaster.c
In file included from /nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/dispatch/dispatch.h:63,
                 from /nix/store/nhf361hkyv2p2cy7nx587sf936vzpvcm-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/CoreFoundation.framework/Heade>
                 from /nix/store/nhf361hkyv2p2cy7nx587sf936vzpvcm-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/CoreFoundation.framework/Heade>
                 from /nix/store/nhf361hkyv2p2cy7nx587sf936vzpvcm-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/CoreFoundation.framework/Heade>
                 from /nix/store/q56y1bbrvdrn4m65bv4r7sxhxg9pifpp-apple-framework-Foundation-11.0.0/Library/Frameworks/Foundation.framework/Headers/Found>
                 from /nix/store/wjpfjap573f8kzc0r7ffabkbj74kmqhz-apple-framework-Cocoa-11.0.0/Library/Frameworks/Cocoa.framework/Headers/Cocoa.h:12,
                 from sysdialog_mac.mm:29:
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/object.h:203:1: error: expected unqualified-id before 'interface'
  203 | OS_OBJECT_DECL_BASE(object, NSObject);
      | ^~~~~~~~~~~~~~~~~~~
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:48:1: error: expected unqualified-id before 'interface'
   48 | OS_WORKGROUP_DECL(os_workgroup, WorkGroup);
      | ^~~~~~~~~~~~~~~~~
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:48:1: error: 'OS_os_workgroup' does not name a type
   48 | OS_WORKGROUP_DECL(os_workgroup, WorkGroup);
      | ^~~~~~~~~~~~~~~~~
In file included from /nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup.h:30,
                 from /nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/dispatch/dispatch.h:64:
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:93:24: error: 'os_workgroup_t' was not declared in this scope;>
   93 | os_workgroup_copy_port(os_workgroup_t wg, mach_port_t *mach_port_out);
      |                        ^~~~~~~~~~~~~~
      |                        os_workgroup_attr_t
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:93:55: error: expected primary-expression before '*' token
   93 | os_workgroup_copy_port(os_workgroup_t wg, mach_port_t *mach_port_out);
      |                                                       ^
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:93:56: error: 'mach_port_out' was not declared in this scope; >
   93 | os_workgroup_copy_port(os_workgroup_t wg, mach_port_t *mach_port_out);
      |                                                        ^~~~~~~~~~~~~
      |                                                        mach_port_t
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:93:69: error: expression list treated as compound expression i>
   93 | os_workgroup_copy_port(os_workgroup_t wg, mach_port_t *mach_port_out);
      |                                                                     ^
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:116:1: error: 'os_workgroup_t' does not name a type; did you m>
  116 | os_workgroup_t _Nullable
      | ^~~~~~~~~~~~~~
      | os_workgroup_attr_t
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:137:1: error: 'os_workgroup_t' does not name a type; did you m>
  137 | os_workgroup_t _Nullable
      | ^~~~~~~~~~~~~~
      | os_workgroup_attr_t
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:177:19: error: 'os_workgroup_t' was not declared in this scope>
  177 | os_workgroup_join(os_workgroup_t wg, os_workgroup_join_token_t token_out);
      |                   ^~~~~~~~~~~~~~
      |                   os_workgroup_join
/nix/store/kk9lnicm474vif1bmhf1268xzrn7z35f-libSystem-11.0.0/include/os/workgroup_object.h:177:64: error: expected primary-expression before 'token_out'
  177 | os_workgroup_join(os_workgroup_t wg, os_workgroup_join_token_t token_out);
      |                                                                ^~~~~~~~~

# ~28k lines of error message ignored 

@yzx9 yzx9 requested a review from nim65s October 27, 2024 12:34
@yzx9 yzx9 force-pushed the feature/openctm-darwin-support branch from 31fb3ef to b5ca35e Compare October 27, 2024 12:42
@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 27, 2024
@yzx9 yzx9 force-pushed the feature/openctm-darwin-support branch from b5ca35e to 6a7220c Compare October 28, 2024 04:49
@yzx9 yzx9 force-pushed the feature/openctm-darwin-support branch from 6a7220c to f9081aa Compare October 28, 2024 05:10
@yzx9
Copy link
Contributor Author

yzx9 commented Oct 28, 2024

4 packages built:
openctm openctm.bin openctm.dev openctm.man

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 351642


aarch64-darwin

✅ 4 packages built:
  • openctm
  • openctm.bin
  • openctm.dev
  • openctm.man

@nim65s
Copy link
Contributor

nim65s commented Oct 28, 2024

Result of nixpkgs-review pr 351642 run on x86_64-linux 1

6 packages built:
  • meshlab
  • openctm
  • openctm.bin
  • openctm.dev
  • openctm.man
  • python311Packages.pymeshlab (python312Packages.pymeshlab)

Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, except if @Aleksanaa has a solution for this gccStdenv issue

@Aleksanaa
Copy link
Member

Nope, it looks not very bad

@Aleksanaa Aleksanaa closed this Oct 28, 2024
@Aleksanaa Aleksanaa reopened this Oct 28, 2024
@ofborg ofborg bot requested a review from nim65s October 28, 2024 15:26
@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Oct 28, 2024
@Aleksanaa Aleksanaa merged commit d0ba2f9 into NixOS:master Oct 29, 2024
34 of 41 checks passed
@yzx9 yzx9 deleted the feature/openctm-darwin-support branch October 29, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants