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

qgis: Enable mapserver #259303

Closed
wants to merge 2 commits into from
Closed

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Oct 6, 2023

Description of changes

Here we enable QGis's internal tileserver implementation. I have found this useful and the size of the binary itself is negligible (around 300kB).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

Copy link
Contributor

@imincik imincik left a comment

Choose a reason for hiding this comment

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

@bgamari , thank you very much for contribution. I am interested in server build too, but it would be much better if we build separate server package. Server package should be built for LTR version as well and nixos test is should be included with PR.

I am happy to help you if you are interested in enhancing your PR.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 6, 2023

@bgamari , thank you very much for contribution. I am interested in server build too, but it would be much better if we build separate server package.

@imincik do you mean a separate package or a separate output? It is not clear to me that QGIS's build system supports building only the server.

@imincik
Copy link
Contributor

imincik commented Oct 6, 2023

do you mean a separate package or a separate output?

That's a good question, I am not sure. Maybe separate server output is good enough. What do you think ?

@bgamari
Copy link
Contributor Author

bgamari commented Oct 7, 2023

That's a good question, I am not sure. Maybe separate server output is good enough. What do you think ?

I suspect that a separate output is really the only viable option. From a cursory inspection of QGIS's build system, it doesn't appear to be possible to build the core library and then later (in a separate tree) link the desktop application/server against that library. Consequently, I think the only option is to build all three and then carefully separate them into separate outputs.

However, I'm not sure I feel confident enough in my knowledge of QGIS's internal structure to be able to safely do this.

@ofborg ofborg bot requested review from imincik and sikmir October 7, 2023 16:54
@bgamari
Copy link
Contributor Author

bgamari commented Oct 7, 2023

I have looked more closely at the output of the derivation and have concluded that I don't feel comfortable performing an output split. There are simply too many files which I, as a relative casual user of QGIS, cannot easily determine the usage of (e.g. should share/qgis/svg belong in the lib or bin output? the same for lib/qgis/plugins/libplugin_geometrychecker.so? lib/qgis/qgiscrashhandler?)

I think either we should merge this as-is or we decide not to merge at all. The risk (and maintainability challenge) of performing an output split strikes me as not justified by the very minor size increase that enabling the server component.

I have, however, integrated @sikmir's suggestion and propagated the change to the LTR derivation.

@imincik
Copy link
Contributor

imincik commented Oct 8, 2023

What about following QGIS Debain packaging and put everything what they in qgis-server-*.install files in to our server output ?

@bgamari
Copy link
Contributor Author

bgamari commented Oct 10, 2023

What about following QGIS Debain packaging and put everything what they in *-server.install files in to our server output ?

I suppose you mean qgis-server-*.install? While I'm not necessarily opposed, I do really question what problem we are solving by doing so. It seems like we are making maintenance work for ourselves for fairly little tangible benefit.

Here we enable QGis's internal [tileserver] implementation. I have found
this useful and the size of the binary itself is negligible (around
300kB).

[tileserver]: https://docs.qgis.org/3.28/en/docs/server_manual
@imincik
Copy link
Contributor

imincik commented Oct 11, 2023

I do really question what problem we are solving by doing so. It seems like we are making maintenance work for ourselves for fairly little tangible benefit.

I think the main reason is to keep QGIS default package size down. I guess that vast majority of QGIS package users are using only desktop interface. Also, I think the list of files belonging to server is not changing very often so the maintenance cost wouldn't be very high.

@imincik
Copy link
Contributor

imincik commented Oct 11, 2023

What about following QGIS Debain packaging and put everything what they in *-server.install files in to our server output ?

I suppose you mean qgis-server-*.install? While I'm not necessarily opposed, I do really question what problem we are solving by doing so. It seems like we are making maintenance work for ourselves for fairly little tangible benefit.

Yes, sorry. I fixed that in my comment.

@imincik
Copy link
Contributor

imincik commented Nov 13, 2023

@bgamari , I am experimenting with QGIS server build in #267301 .

@imincik imincik mentioned this pull request Nov 15, 2023
13 tasks
@imincik
Copy link
Contributor

imincik commented Nov 15, 2023

This PR is now replaced by #267301

@imincik imincik closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants