Zandronum Chat @ irc.zandronum.com
#zandronum
Get the latest version: 3.0
Source Code

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003538Doomseeker[All Projects] Bugpublic2018-10-06 11:492018-10-27 22:54
ReporterWubTheCaptain 
Assigned ToZalewa 
PrioritynormalSeveritytweakReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version1.1 
Target Version1.2Fixed in Version1.2 
Summary0003538: The preferred form of IP2C database for modifications (CSV) is not currently distributed
DescriptionPolicies or licenses (GPL) may require conveying the source code to all object code distributed. How this should be resolved with IpToCountry.dat creates some interesting debates/issues:

0003512:0019932:

Quote from Zalewa
I don't like an approach where the database would be downloaded from MaxMind and converted on the fly during the package process. Not only this adds a mandatory dependency on Python, it also prevents making deterministic builds (counters 0003255). Moreover, it will mean that we will either have to get rid of the "IP2C Update" feature from the program or automatize update of the IpToCountry.dat available from our home page or otherwise it may turn out that the preinstalled database is more up-to-date than the supposedly up-to-date database on the home page.

Additional InformationIpToCountry.dat is the IP2C database object used by Doomseeker, built from CSV source with geolite2_conv.py (which currently also downloads the CSV source from MaxMind).

Downloading something during the build process would also likely be a Debian policy violation.
Attached Files

- Relationships
related to 0003255confirmed Support reproducible, deterministic builds 
related to 0003537closedZalewa Unbake IpToCountry.dat from executable (resources.qrc), make it optional 
child of 0003246confirmedWubTheCaptain Debian packaging 
child of 0003512closedZalewa MaxMind GeoLite2: Update from CC BY-SA 3.0 licensed dataset to newer CC BY-SA 4.0 licensed work 

-  Notes
User avatar (0019936)
WubTheCaptain (developer)
2018-10-06 11:53

0003512:0019932:

Quote from Zalewa
We could avoid putting 13 useless Megabytes into the repository by packaging the MaxMind's CSV files together with our IpToCountry.dat file in an archive and putting that archive on our home page and stating in compilation instructions that to prepare the proper deployment package you need to grab this package from the server. The current CMake INSTALL() dependency on IpToCountry.dat will have to be made optional. Negative consequences of this are that a) the build process gets more complicated, and b) the risk of preparing the package incorrectly rises. So, this is still something that I don't like.

Finally, we reach upon the solution where we put the 13 Megabytes of CSV together with the IpToCountry.dat prepared from that exact CSV into the repo in a singular commit. This should satisfy the requirements of "preferable edit format" and also keep in check the exact license that the currently preinstalled database is on (MaxMind's archive contains the license file).
User avatar (0019939)
WubTheCaptain (developer)
2018-10-06 12:19

Here's my opinions:

Quote from Zalewa
I don't like an approach where the database would be downloaded from MaxMind


I don't like this.

Quote from Zalewa
converted on the fly during the package process


I like this (for Debian GNU/Linux), especially because it takes only few seconds now since commit 536f21e02e30fb552fcdb8e832b400064bc56fb9.

Quote from Zalewa
Not only this adds a mandatory dependency on Python


I don't like this much. Ok if the this is a seperate (source) package, e.g. doomseeker-ip2c.

Quote from Zalewa
Moreover, it will mean that we will either have to get rid of the "IP2C Update" feature from the program


Unnecessary, if there's one specific version of the CSV database in each version? Though personally, I believe in less features being better.

Quote from Zalewa
or automatize update of the IpToCountry.dat available from our home page


See above.

Quote from Zalewa
or otherwise it may turn out that the preinstalled database is more up-to-date than the supposedly up-to-date database on the home page.


This may theoretically happen at any time, anyway.

Quote from Zalewa
We could avoid putting 13 useless Megabytes into the repository by packaging the MaxMind's CSV files together with our IpToCountry.dat file in an archive and putting that archive on our home page and stating in compilation instructions that to prepare the proper deployment package you need to grab this package from the server.


I'm liking this.

If IpToCountry.dat is included with GeoLite2-Country-CSV.zip (or its contents), there will be a DFSG-removal of IpToCountry.dat in Debian but alright.

Quote from Zalewa
The current CMake INSTALL() dependency on IpToCountry.dat will have to be made optional.


I'm possibly liking this.

Quote from Zalewa
Negative consequences of this are that a) the build process gets more complicated, and b) the risk of preparing the package incorrectly rises. So, this is still something that I don't like.


As far as I've understood this is not a concern, just like Doomseeker and Wadseeker are seperate packages and one depends on the other.

Quote from Zalewa
Finally, we reach upon the solution where we put the 13 Megabytes of CSV together with the IpToCountry.dat prepared from that exact CSV into the repo in a singular commit. This should satisfy the requirements of "preferable edit format" and also keep in check the exact license that the currently preinstalled database is on (MaxMind's archive contains the license file).


Yes, but create a new repository for this for CSV / IP2C updates only.
User avatar (0019940)
WubTheCaptain (developer)
2018-10-06 12:20

Quote from Zalewa
Not only this adds a mandatory dependency on Python


Oh and well, I guess there's always an option to rewrite the tool in C or C++.
User avatar (0019941)
WubTheCaptain (developer)
2018-10-06 12:24

And my point here is: You can keep building Doomseeker from source during development, but make IP2C truly optional. This would mean a new repository for IP2C data and source, and this may be updated less frequently (or autoupdated with IP2C Update).

The only concern here I see is setting a stable date/revision for the IP2C database.
User avatar (0019945)
Zalewa (developer)
2018-10-06 13:47
edited on: 2018-10-06 13:56

Quote from WubTheCaptain

I believe in less features being better.

But you do not believe in complicating the building process being worse?

* Increasing amount of text in COMPILE.Windows.txt and COMPILE.txt by another section is complicating the build as someone now needs to read more to be ready to do work on the project.

* Moving IP2C data to a separate repository is complicating the build. I loathe projects that feel the need to split everything into tiny repositories, especially when it turns out you need to alter something and there are interdependencies between the repositories and you need to simultaneously make changes and push commits to both.

* Optional INSTALL() statement is complicating the build on the basis that it allows the person who prepares the package to make a mistake and not include the item that was meant to be included. Programmers create software to remove the possibility of human error, not enable it, and Doomseeker should really always be redistributed with the database.

* Even though we can safely assume that every Linux distro will have a Python interpreter available, Windows needs to have it installed separately. I have Python on all my Windows machines because it is useful to have it, but not providing a premade IpToCountry.dat for any other potential contributor who would also want to use Windows is complicating the build and reducing this contributor's desire to engage with the project.


In regards to the "IP2C Update" feature I'm however beginning to agree with you that this feature has become unnecessary - or rather, since Linux is covered by package managers and Mac & Windows are covered by Mendeley's Updater we can remove the separate "IP2C Update" feature altogether and rely on the aforementioned methods of update.

Quote from WubTheCaptain

Doomseeker and Wadseeker are seperate packages

True for Linux, false for Windows. The default package for Windows is monolithic and includes the whole distribution with all plugins, Qt, MSVC redist and docs. The only other Windows package in existence I know of is the one that's bundled with Zandronum and it's pretty much the same other than lacking non-Zandronum plugins.

Quote from WubTheCaptain

Quote from Zalewa

Negative consequences of this are that a) the build process gets more complicated, and b) the risk of preparing the package incorrectly rises.

As far as I've understood this is not a concern

Complicating a build is pretty much always a concern. Look how simple building Doomseeker on Linux is right now - you get the repo, satisfy the dependencies through the package manager and it will build without any fuss.

If the problem we're having here was caused by a technical (software engineering) issue or solving it was needed to implement some new feature I would relent and gladly complicate the build as the end-user would benefit from it. But let's face it: we're spending lots of resources to solve a problem that average user doesn't give a damn about, we've been in violation of the license for a long time now and no one cared and we're introducing contrivances to solve issues that exist only on paper and need to be explained by even more paper.

Unless someone has less contrived idea, the only solution that I dislike the least is putting the CSVs into the current repo. By this I also wish to state that I do not like any of the solutions here, but since we want to comply with licensing as best as we can, I'm willing to choose the lesser of several evils to solve the problem.

User avatar (0019948)
WubTheCaptain (developer)
2018-10-06 14:41
edited on: 2018-10-06 14:41

Quote from Zalewa
But you do not believe in complicating the building process being worse?


It's a packaging issue, not a software one. Windows "does not" have a good package manager available. Of course it's worse.

Heck, I'd prefer if there was no CMake (or any GNU autohell) involved in building Doomseeker; it'd be quite unrealistic to expect of a Qt software not UNIX-style or POSIX-like working with standard input/output.

Quote from Zalewa
Increasing amount of text in COMPILE.Windows.txt and COMPILE.txt by another section is complicating the build as someone now needs to read more to be ready to do work on the project.


This should be (or needs to be) done anyway, for someone wanting to build Doomseeker truly from scratch using the source, minus the "ready to do work" part. But the IP2C is mainly optional for a source hacker testing changes...

Quote from Zalewa
I loathe projects that feel the need to split everything into tiny repositories, especially when it turns out you need to alter something and there are interdependencies between the repositories and you need to simultaneously make changes and push commits to both.


Agreed. But why would you need to push anything but upstream CSV data and/or built IpToCountry.dat there? This happens quite infrequently, anyway.

Quote from Zalewa
I have Python on all my Windows machines because it is useful to have it, but not providing a premade IpToCountry.dat for any other potential contributor who would also want to use Windows is complicating the build and reducing this contributor's desire to engage with the project.


The problem is not with including a premade IpToCountry.dat. You can include it, ship .exe along with it and offer/convey the source code (to Doomseeker, not necessarily IpToCountry.dat).

Only Debian Free Software Guidelines (DFSG) see it as an issue. There can be no precompiled files in Debian's source tarball (derived from upstream tarball, and named a DFSG-source if modified by the package maintainer with those generated files removed). If no source is available at all, that will most likely block creating a package to the free repositories (thus, the 13MB CSV argument).

Quote from Zalewa
Optional INSTALL() statement is complicating the build on the basis that it allows the person who prepares the package to make a mistake and not include the item that was meant to be included.


Make it (loudly) state during cmake if internal or system database will be used? This is what we already with other dependencies.

Oh, and we have a proposal to make the game/engine plugins optional too.

Quote from Zalewa
Even though we can safely assume that every Linux distro will have a Python interpreter available


Fyi: OpenBSD – although not GNU/Linux – does not have Python (but there's a port available). Anyway...

Quote from Zalewa
In regards to the "IP2C Update" feature I'm however beginning to agree with you that this feature has become unnecessary - or rather, since Linux is covered by package managers and Mac & Windows are covered by Mendeley's Updater we can remove the separate "IP2C Update" feature altogether and rely on the aforementioned methods of update.


New issue 0003541.

Quote from Zalewa
True for Linux, false for Windows.


0003292 looks ever so much more inviting to resolve, anyway we're not making progress with this ticket.

Quote from Zalewa
the only solution that I dislike the least is putting the CSVs into the current repo


As long as there is a tool to convert from this file without downloads from the Internet, it should be okay for Debian.

I agree I don't like the solution either, but it's also the one I dislike the least too.

User avatar (0019949)
WubTheCaptain (developer)
2018-10-06 14:41

So, is the solution here to include a revision of the CSV files in Doomseeker/doomseeker repository?
User avatar (0019957)
Zalewa (developer)
2018-10-06 17:35

Quote from WubTheCaptain

So, is the solution here to include a revision of the CSV files in Doomseeker/doomseeker repository?

I'd say yes. Blzut?
User avatar (0019970)
Blzut3 (administrator)
2018-10-06 21:28

I think the fact being missed here is that presumably the reason that we baked an IP2C database into the executable in the first place is to smooth out the start up experience. In other words we want users to immediately see flags instead of having to wait for a download to complete. To that end, my assumption the whole time here is that our IP2C updater would provide the canonical source of that database whether a database is included with the program (our distributions) or not (Debian). If we really wanted to adding a timestamp to the database would be an easy way to solve the "local is newer" problem (this timestamp could be put in the HTTP header when checking for a newer database), but I personally don't think it's required to be solved.

Since having the initial database is not a requirement, there's no additional hard dependency on Python. Just an optional one for those creating release packages. Those just building for themselves won't care since they'll just download the database when the app starts.

Another thing to keep in mind is that if we continue to include a database in the source, how long until Wub opens a ticket every week to tell us to update the vendored database. :P

As for build reproducibility: This is a fair point, but as an optional component that contains highly volatile data I'm not sure it's particularly important to ensure that the exact state of the database at release time is reproducible. Keep in mind also that the usefulness of an old version of Doomseeker also falls apart when protocols change, so this project is kind of temporal in nature.

Complicating the packaging process: I already have a script for Mac (makemacdmg.sh/mergemacdmg.sh), Linux (cpackpostprocess.sh), and sources packages (makesourcepackages.sh). Inserting a database into the zip file could easily be captured here. You might even be able to use "cmake -E tar --format=zip" to do this without having to assume the user has a zip tool installed. (Although I wouldn't be surprised if Powershell just has this built in.)

In short I think I'd rather continue to update IP2C databases the way we are now, especially in light of having to disable the Mac program auto updates. If including a database in the repo makes you sleep better at night then do it. I just think it's a pointless tens of MB. Perhaps reconsider the idea of a separate repo for the database if your primary concern is having the databases frozen in time (again it's optional so no inter-dependencies), but I'm not going to lose sleep if you insist on keeping it in the main repo.
User avatar (0019985)
Zalewa (developer)
2018-10-07 09:38

In spite of my concerns I have finally decided to move the IP2C data to the "blobs" repository which can be used in the future to insert there more big binary stuff (though with much, much caution and reluctance, please).

CSVs were put into this repository in this particular commit:https://bitbucket.org/Doomseeker/doomseeker-blobs/commits/c33890cb9a0f39a17a0a0369a1f28f202d2b79e4 [^]

I have not included the IpToCountry.dat blob because it can be quickly generated using the geolite2_conv.py script, which also now resides in the blobs repo. To save myself from remembering the command line, I also allowed myself to add a make_ip2c.bat file that will prepare the files as we need them. geolite2_conv.py has also been modified to create 'md5' and 'geolite2.gz' files with hardcoded names that can be directly uploaded through FTP to our website for "IP2C Update" purpose (0003541).

As IpToCountry.dat has been removed from Doomseeker's main repo it has become necessary to introduce DOOMSEEKER_IP2C_DAT CMake option:https://bitbucket.org/Doomseeker/doomseeker/commits/ad48cade47f154f56970ecf117a8306748c08199 [^]

And, as a final consequence, since IpToCountry.dat inclusion in the distribution doesn't "just work" anymore, it became necessary to explain how to include it in the compilation instructions:https://bitbucket.org/Doomseeker/doomseeker/commits/13bf2040a055181d3403a575a489701c03f214bc [^]


Finally, the excessive granularity of these tickets makes it difficult to attach the correct ticket number to the commit message and forces me to repeat myself in these notes.
User avatar (0019992)
WubTheCaptain (developer)
2018-10-07 11:03

We'll see how this goes for packaging in the future, and open issues later.

Quote from Zalewa
Finally, the excessive granularity of these tickets makes it difficult to attach the correct ticket number to the commit message and forces me to repeat myself in these notes.


Sorry, I believe this was a necessary evil to divulge into some topics more in-depth without making the notes look noisy and off-topic to the issue mentioned in OP (and assumed resolution). In case of doubt, the correct ticket number for commit messages is usually the parent ticket.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2018-10-06 11:49 WubTheCaptain New Issue
2018-10-06 11:49 WubTheCaptain Summary IP2C database is downloaded from MaxMind and converted on the fly during the package process => IP2C database "is" downloaded from MaxMind and converted on the fly during the package process
2018-10-06 11:50 WubTheCaptain Relationship added child of 0003246
2018-10-06 11:50 WubTheCaptain Relationship added related to 0003512
2018-10-06 11:51 WubTheCaptain Target Version => 1.2
2018-10-06 11:51 WubTheCaptain Description Updated View Revisions
2018-10-06 11:51 WubTheCaptain Relationship added related to 0003255
2018-10-06 11:53 WubTheCaptain Note Added: 0019936
2018-10-06 11:58 WubTheCaptain Product Version => 1.1
2018-10-06 12:00 WubTheCaptain Relationship added related to 0003537
2018-10-06 12:01 WubTheCaptain Relationship deleted related to 0003512
2018-10-06 12:03 WubTheCaptain Status new => confirmed
2018-10-06 12:19 WubTheCaptain Note Added: 0019939
2018-10-06 12:20 WubTheCaptain Note Added: 0019940
2018-10-06 12:20 WubTheCaptain Summary IP2C database "is" downloaded from MaxMind and converted on the fly during the package process => IP2C database would need to be downloaded from MaxMind and converted on the fly during the package process
2018-10-06 12:24 WubTheCaptain Note Added: 0019941
2018-10-06 12:30 WubTheCaptain Description Updated View Revisions
2018-10-06 12:31 WubTheCaptain Description Updated View Revisions
2018-10-06 12:36 WubTheCaptain Description Updated View Revisions
2018-10-06 12:37 WubTheCaptain Summary IP2C database would need to be downloaded from MaxMind and converted on the fly during the package process => The preferred form of IP2C database for modifications (CSV) is not currently distributed
2018-10-06 12:37 WubTheCaptain Description Updated View Revisions
2018-10-06 12:37 WubTheCaptain Additional Information Updated View Revisions
2018-10-06 12:37 WubTheCaptain Relationship added child of 0003512
2018-10-06 12:38 WubTheCaptain Relationship replaced related to 0003512
2018-10-06 12:38 WubTheCaptain Relationship added related to 0003237
2018-10-06 12:40 WubTheCaptain Description Updated View Revisions
2018-10-06 12:40 WubTheCaptain Additional Information Updated View Revisions
2018-10-06 12:41 WubTheCaptain Relationship deleted related to 0003237
2018-10-06 12:42 WubTheCaptain Relationship replaced child of 0003512
2018-10-06 13:47 Zalewa Note Added: 0019945
2018-10-06 13:56 Zalewa Note Edited: 0019945 View Revisions
2018-10-06 14:41 WubTheCaptain Note Added: 0019948
2018-10-06 14:41 WubTheCaptain Note Edited: 0019948 View Revisions
2018-10-06 14:41 WubTheCaptain Note Added: 0019949
2018-10-06 17:35 Zalewa Note Added: 0019957
2018-10-06 18:06 WubTheCaptain Assigned To => WubTheCaptain
2018-10-06 18:06 WubTheCaptain Status confirmed => feedback
2018-10-06 18:06 WubTheCaptain Assigned To WubTheCaptain =>
2018-10-06 21:28 Blzut3 Note Added: 0019970
2018-10-07 06:17 Zalewa Assigned To => Zalewa
2018-10-07 06:17 Zalewa Status feedback => assigned
2018-10-07 09:38 Zalewa Note Added: 0019985
2018-10-07 09:38 Zalewa Status assigned => needs review
2018-10-07 11:03 WubTheCaptain Note Added: 0019992
2018-10-07 11:03 WubTheCaptain Status needs review => resolved
2018-10-07 11:03 WubTheCaptain Fixed in Version => 1.2
2018-10-07 11:03 WubTheCaptain Resolution open => fixed
2018-10-27 22:54 WubTheCaptain Status resolved => closed






Questions or other issues? Contact Us.

Links


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker