Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)
http://www.rttr.info
GNU General Public License v2.0
476 stars 75 forks source link

Update nix shell to current repository eco system. #1511

Closed Shawn8901 closed 2 years ago

Shawn8901 commented 2 years ago

The shell.nix file which was added to the project is a quite bit outdated and does not result in a runtime which is capable of compiling the game. As i am currently running nixos (which is a distro build around the package manager nix, which at the end, the shell.nix can possibly be used in), i gave it a try to update it to the current state of the nix eco system.

The old version was mostly broken due to not having a SDL2 which includes a libSDL2Main.a, thus the cmake modules do not find the needed version information.

edit: sorry for force pushing again. but after cleaning old devShells i noticed that it also requires libsamplerate which was because of some reason on the shell before by something else.

Flamefire commented 2 years ago

LGTM. As I don't have any environment to test this: Have you tried this in the current state if it works?

Shawn8901 commented 2 years ago

Yes it results in an environment which can compile the game.

Only open topic, but that was open since the shell.nix or the test has been added: Test_rttrConfig fails, as its relying on the current system date on build time. Nix build system is statically setting that to 19800101, which does fail the test as its checking for a year >2000. And i don't see any possibility to change that behavior, at the moment.

Flamefire commented 2 years ago

Only open topic, but that was open since the shell.nix or the test has been added: Test_rttrConfig fails, as its relying on the current system date on build time. Nix build system is statically setting that to 19800101, which does fail the test as its checking for a year >2000. And i don't see any possibility to change that behavior, at the moment.

Why is that? Doesn't seem to really make sense to me.

FWIW: The test is this: https://github.com/Return-To-The-Roots/s25client/blob/397f2b2315e997504d4958bfbdea0af815ce559a/tests/rttrConfig/testRttrConfig.cpp#L68 and https://github.com/Return-To-The-Roots/s25client/blob/397f2b2315e997504d4958bfbdea0af815ce559a/tests/rttrConfig/testRttrConfig.cpp#diff-06081d187998cf02cc78e8743ff625930f76a9cd6323bee5d2820e862ddadc1aR78 So you may add a patch there

We set that at https://github.com/Return-To-The-Roots/s25client/blob/a24429ffa9426557304144d523729013da3799f4/libs/rttrConfig/CMakeLists.txt#L25 but I don't see any worth in changing this or the test as everything is working as intended and a "wrong" system time is outside the scope of that. So the Nix guys would need to figure out how to pass the build date/time to the build system (i.e. CMake)

Shawn8901 commented 2 years ago

Why is that? Doesn't seem to really make sense to me.

When focusing reproducible builds a build should not to rely on things like systems epoch, as re-building the same code to a later time does produce different outputs, so it will not produce the same binary and so its not reproducible. RTTR does not focus that, as its using systems epoch at least on the version string for non-release builds (which is of course not bad, by itself).

A better behavior from the idea of having builds reproducible, the code could use git commits date, so a build today has the exact same output like a fresh build tomorrow, if its on the same commit. Maybe that does give a bit additional context: https://reproducible-builds.org/specs/source-date-epoch/

As cmake is aware of such things, and in nix the epoch is set to 315532800 (which is a deterministic non 0 value => 1980-01-01) if no other source is given, we did not pass the sanity check done by the unit test. As I did some research where nix is setting that magic value l found that spec (before i did just know why) i also found the solution for the build date used for non release builds and also for the unit test. When setting SOURCE_DATE_EPOCH to the commits date, which can be done in the shell.nix both issues are addressed: The programmer has a build date (ok for nix its the commit date in that case, but at least reproducible), and the unit test passes, as the commit date passes the sanity check.

but I don't see any worth in changing this or the test as everything is working as intended and a "wrong" system time is outside the scope of that. So the Nix guys would need to figure out how to pass the build date/time to the build system (i.e. CMake)

I agree that its not the goal to patch that out of rttrConfig-Test nor of the cmake definition. As its not per definition nix specific but reproducible build specific cmake luckily is already aware of such a thing if i had known that before... :)

At the end i just want to repeat, that this has been an issue before, so its likely that the person who has added the shell.nix either did never run the tests, the test was added later, or the person did not care about the failing test, of course at the end its not important what went wrong, as its fixed now. :+1:

edit: sorry for force pushing again which interrupts the build...

Flamefire commented 2 years ago

A better behavior from the idea of having builds reproducible, the code could use git commits date,

The problem with this is that this only works with git checkouts, not when the source code has been obtained/transferred in other means. E.g. the git tags on Github create source tarballs and we add an own one which includes the submodules, see e.g. https://github.com/Return-To-The-Roots/s25client/releases/tag/v0.9.5
It also requires to have git installed, which is minor of course.

Anyway: Is it possible, to provide a fallback value? I.e. if the git command fails for any reason (git not found, not a git repo, ...) use some default with a year >= 2000, e.g. 2005 (same as the copyright notices)

Shawn8901 commented 2 years ago

edit: Using the updateSourceDateEpoch shell hook (magic in the nix repository to set that epoch to the newest file in a folder) looks like the most desired for all sides.