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

[cspice] Download 64 bits packages if targeting 64 bits arch + NON_UNIX_STDIO definition for UNIX #9788

Closed
SpaceIm opened this issue Jan 22, 2020 · 0 comments · Fixed by #12903
Assignees
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist

Comments

@SpaceIm
Copy link

SpaceIm commented Jan 22, 2020


Is your feature request related to a problem? Please describe.

  1. NASA NAIF provides one package per os/arch/compiler. There are several differences in the source code between those packages. Currently, cspice port downloads the 32 bits package of the detected os, whatever the targeted architecture.
    It has to be notice that underlying type of two cspice types (SpiceInt and ConstSpiceInt) are different between 32 bits and 64 bits source code (long vs int, which shouldn't have any consequences for most compilers I guess, but why playing with the devil?).
    And for non Windows, there are underlying type of similar fortran types which are different.
    It's also strange that compile option -m64 is hardcoded in CMakeLists.txt for UNIX.

Just an example of code diff (here between VisualC-32bit and VisualC-64bit packages):

--- a/include/SpiceZpl.h
+++ b/include/SpiceZpl.h
@@ -111,7 +111,7 @@
 #define HAVE_PLATFORM_MACROS_H
  
  
-   #define   CSPICE_PC_MS
+   #define   CSPICE_PC_64BIT_MS
  
 #endif
  
--- a/src/cspice/SpiceZpl.h
+++ b/src/cspice/SpiceZpl.h
@@ -111,7 +111,7 @@
 #define HAVE_PLATFORM_MACROS_H
  
  
-   #define   CSPICE_PC_MS
+   #define   CSPICE_PC_64BIT_MS
  
 #endif
  
--- a/src/cspice/zzplatfm.c
+++ b/src/cspice/zzplatfm.c
@@ -348,7 +348,8 @@ static integer c__6 = 6;
 
 	s_copy(attcpy + 32, "PC", (ftnlen)32, (ftnlen)2);
 	s_copy(attcpy + 64, "MICROSOFT WINDOWS", (ftnlen)32, (ftnlen)17);
-	s_copy(attcpy + 96, "MICROSOFT VISUAL C++", (ftnlen)32, (ftnlen)20);
+	s_copy(attcpy + 96, "MICROSOFT VISUAL C++/64BIT", (ftnlen)32, (ftnlen)
+		26);
 	s_copy(attcpy + 128, "LTL-IEEE", (ftnlen)32, (ftnlen)8);
 	s_copy(attcpy + 160, "CR-LF", (ftnlen)32, (ftnlen)5);
 	s_copy(attcpy + 192, "BIG-IEEE LTL-IEEE", (ftnlen)32, (ftnlen)17);
  1. For Unix: NON_UNIX_STDIO should be defined, and NON_ANSI_STDIO shouldn't (based on build scripts included in official cspice packages).

Proposed solution

  1. Check architecture in portfile.cmake and download 32 bits or 64 bits cspice package. In CmakeLists.txt, if UNIX, in target_compile_options use -m32 or -m64 based on arch (is it not automatically injected by vcpkg?).
  2. In CMakeLists.txt:
    • replace target_compile_definitions(cspice PUBLIC "NON_ANSI_STDIO") by target_compile_definitions(cspice PUBLIC "NON_UNIX_STDIO")
    • remove mktemp.patch (modification of dead code if NON_ANSI_STDIO not defined)
@SpaceIm SpaceIm added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants