-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Build QGIS and dependencies on darwin #36902
Conversation
ifneq ($(findstring darwin,$(ARCH)),) | ||
@# enable OSX Help Viewer | ||
- @/bin/ln -sfh "$(INST_DIR)/docs/html" /Library/Documentation/Help/GRASS-$(GRASS_VERSION_MAJOR).$(GRASS_VERSION_MINOR) | ||
+ #@/bin/ln -sfh "$(INST_DIR)/docs/html" /Library/Documentation/Help/GRASS-$(GRASS_VERSION_MAJOR).$(GRASS_VERSION_MINOR) |
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.
Is this commenting it out? If so, maybe prefer just removing it completely.
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.
Yes it is commented out. I can remove it completely as well, I was just trying to make the least change possible.
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 would just remove it, that makes it much easier to read the diff.
fcgi libspatialindex libspatialite postgresql qjson qca2 txt2tags ] ++ | ||
fcgi libspatialindex libspatialite postgresql qjson qca2 txt2tags pkgconfig | ||
(stdenv.lib.optional stdenv.isDarwin darwin.apple_sdk.frameworks.IOKit) | ||
(stdenv.lib.optional stdenv.isDarwin darwin.apple_sdk.frameworks.ApplicationServices) |
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.
Maybe append lib.optionals stdenv.isDarwin (with darwin.apple_sdk.frameworks; [IOKit ApplicationServices])
instead of having these individual entries
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.
Extending the callPackage scope in all-packages.nix
with frameworks is the most common approach.
++ stdenv.lib.optional stdenv.isDarwin | ||
(["-DCMAKE_FIND_FRAMEWORK=never"] | ||
++ ["-DQGIS_MACAPP_BUNDLE=0"]); | ||
# ++ ["-DCMAKE_BUILD_TYPE=RelWithDebInfo"]; |
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.
Was this left in intentionally?
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 was unsure whether to leave it in or not as it's very useful to see the terminal output when running the application. Like I said, the derivation is currently not perfect but much better than what is currently in the repo.
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.
The release type affects the terminal output? 🤦♂️ ah well, maybe add a comment clarifying how it's useful then
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.
Well it's a graphical application. Building with debug info makes the terminal output very verbose as you use the gui.
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.
No, the actual thing that debug info refers to is the debugging symbols that map the machine code sections back to source code. Terminal output should not usually be affected, but it's up to the application developer whether that's actually the case.
@@ -68,7 +68,7 @@ stdenv.mkDerivation rec { | |||
wrapPythonPrograms | |||
''; | |||
|
|||
enableParallelBuilding = true; | |||
enableParallelBuilding = false; |
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.
Why?
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.
There were files missing in the installation before I set it to false
.
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.
This is worth documenting in a comment and/or the commit message.
wrapProgram $out/bin/qgis \ | ||
--prefix PYTHONPATH : $PYTHONPATH \ | ||
--prefix LD_LIBRARY_PATH : ${stdenv.lib.makeLibraryPath [ openssl ]} | ||
'') ++ |
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.
Currently evaluation fails as shown by our dear GrahamcOfBorg — strings are concatenated with +
, not ++
. Once this is fixed I can get ofborg to try building it :)
@GrahamcOfBorg build grass qgis |
Failure on x86_64-darwin (full log) Attempted: grass, qgis Partial log (click to expand)
|
The build takes over an hour locally on my machine so that time out seems expected. |
Failure on aarch64-linux (full log) Attempted: grass, qgis Partial log (click to expand)
|
Failure on x86_64-linux (full log) Attempted: grass, qgis Partial log (click to expand)
|
I rebased and applied the changes from the suggestions. I'm rebuilding now locally to verify that they work correctly. |
2beffc0
to
2f7dcdf
Compare
postInstall = | ||
(stdenv.lib.optionalString stdenv.isLinux '' | ||
wrapProgram $out/bin/qgis \ | ||
--prefix PYTHONPATH : $PYTHONPATH \ |
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.
use of prefix
allows leaking in PTYTHONPATH. Typically it is better to use set
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.
It was like this before? Was it wrong before?
nativeBuildInputs = [ unzip ] ++ (if withQt5 then [ qmake ] else [ qmake4Hook ]); | ||
|
||
# Fix Xcode 8 compilation problem | ||
xcodePatch = fetchurl { url = "https://raw.githubusercontent.com/Homebrew/formula-patches/a651d71/qscintilla2/xcode-8.patch"; |
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.
either add this directly in the patches
list, or put it in a let in
expression.
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.
Good idea.
I made the two changes @FRidh suggested. |
There are still some problems with the withGrass option but the package is mostly functional.
I rebased onto staging. Thanks @copumpkin |
@grahamc I think borg got confused about the base branch switch somehow 😄or at least I hope this thing doesn't affect the stdenv! @mpickering thanks! |
@copumpkin I am returning to this issue now looking at what is causing |
The source seems to be when compiling in the |
No real ideas, sorry! |
@mpickering actually needed qgis on Darwin today and it worked flawlessly, thanks again for fixing this! |
Motivation for this change
QGIS didn't build on darwin but now it does. I have only tested these patches on OSX. I'm not sure the original QGIS derivation worked properly on nixos anyway, is anyone using it?
It would be good to test the following attributes using @GrahamcOfBorg
grass qgis
The rest should be dependencies of qgis.
cc @viric
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)