Skip to content

Conversation

@Biswa96
Copy link
Contributor

@Biswa96 Biswa96 commented Mar 31, 2022

No description provided.

@scottfurry
Copy link
Contributor

I agree with the idea. I have a bit of concern about execution.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ad57a86b5..da03f6d83 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -530,7 +530,7 @@ if(WIN32 AND MSVC)
     endif()
 endif()
 
-if(NOT WIN32 AND NOT APPLE)
+if((NOT WIN32 AND NOT APPLE) OR MINGW)
     install(TARGETS ${PROJECT_NAME}
         RUNTIME DESTINATION bin
         LIBRARY DESTINATION lib

The logic of change to line 533 leaves possibility of Unix, Win64, Linux, et al.
I'm unsure how best to fix this but I'm leery of the change suggested.

@Biswa96
Copy link
Contributor Author

Biswa96 commented Mar 31, 2022

The logic of change to line 533 leaves possibility of Unix, Win64, Linux, et al.

Doesn't previous logic also include those possibility? There is no Win64. WIN32 includes Win64. 1

There is another logic that can be used. Does this look good?

@@ -530,7 +530,7 @@ if(WIN32 AND MSVC)
     endif()
 endif()

-if(NOT WIN32 AND NOT APPLE)
+if(NOT MSVC AND NOT APPLE)
     install(TARGETS ${PROJECT_NAME}
         RUNTIME DESTINATION bin
         LIBRARY DESTINATION lib

Footnotes

  1. https://cmake.org/cmake/help/latest/variable/WIN32.html

@scottfurry
Copy link
Contributor

The logic of change to line 533 leaves possibility of Unix, Win64, Linux, et al.

Doesn't previous logic also include those possibility? There is no Win64. WIN32 includes Win64. 1

There is another logic that can be used. Does this look good?

@@ -530,7 +530,7 @@ if(WIN32 AND MSVC)
     endif()
 endif()

-if(NOT WIN32 AND NOT APPLE)
+if(NOT MSVC AND NOT APPLE)
     install(TARGETS ${PROJECT_NAME}
         RUNTIME DESTINATION bin
         LIBRARY DESTINATION lib

Footnotes

1. https://cmake.org/cmake/help/latest/variable/WIN32.html [leftwards_arrow_with_hook](#user-content-fnref-1-00f14395ee875d1af3c14bc972778015)

A little further up in CMake docs is Variables That Describe the System.

MSVC is implying a MS Visual Studio Build. This may imply a Windows environment but is not exclusively a Windows system, IIRC.

I think what may be best is that rather than an exclusive condition coupled with an inclusive condition (i.e. (NOT a and NOT b) or C) the logic test should just be simply exclusive only (i.e. NOT a and NOT b and NOT...) or inclusive only (i.e. a or b or c). Mixing seems bad to me, IMHO.

@Biswa96
Copy link
Contributor Author

Biswa96 commented Mar 31, 2022

My apology, I am out of ideas 😔

@justinclift
Copy link
Member

Hmmm, is this a situation where we are potentially rejecting a reasonable PR because the existing approach is too wide open?

Looking at the suggested change, it seems to only add MINGW to the list of environments that would be allowed by the clause, yeah?

@Biswa96
Copy link
Contributor Author

Biswa96 commented Mar 31, 2022

it seems to only add MINGW to the list of environments that would be allowed by the clause

Right.

@longnguyen2004
Copy link
Contributor

The logic of change to line 533 leaves possibility of Unix, Win64, Linux, et al.

I think that's good though, how does that leave out Unix and stuff, since the (NOT WIN32 AND NOT APPLE) clause is still true for those?

@mgrojo
Copy link
Member

mgrojo commented Jun 28, 2022

@Biswa96, @longnguyen2004, would this be compatible/consistent with #3068?

@longnguyen2004
Copy link
Contributor

Yes it's fine, this is for install directories, and the other one is for better dependency management.

@mgrojo mgrojo merged commit 3c8af7a into sqlitebrowser:master Sep 14, 2022
@mgrojo
Copy link
Member

mgrojo commented Sep 14, 2022

Thanks, @longnguyen2004, and sorry for the long delay.

@Biswa96 Biswa96 deleted the cmake-mingw-install branch September 15, 2022 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants