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

modelcompiler sometimes fails to build any .sgm files. #4730

Closed
orbea opened this issue Nov 1, 2019 · 21 comments
Closed

modelcompiler sometimes fails to build any .sgm files. #4730

orbea opened this issue Nov 1, 2019 · 21 comments

Comments

@orbea
Copy link
Contributor

orbea commented Nov 1, 2019

Observed behaviour

If pioneer is installed to the system then the next time pioneer is built modelcompiler will skip creating any .sgm files which already exist on the system.

When creating distro packages this is very problematic because the created packages are not reproducible if they are already installed.

The problem seems to be somewhere around here where ends_with_ci will never be true and the .sgm files are never created.

if (ends_with_ci(fpath, ".model")) { // store the path for ".model" files

Edit:

Its returning false in ends_with_ci here.

if (tolower(*s++) != tolower(*t++)) return false;

Expected behaviour

Pioneer should always generate .sgm files during the build.

Steps to reproduce

  1. Build and install pioneer using -DPIONEER_DATA_DIR=/usr/share/pioneer.
    2, Now rebuild pioneer with a fresh build directory using the same directory as in the previous step, -DPIONEER_DATA_DIR=/usr/share/pioneer.
  2. The modelcompiler will skip creation of any .sgm files since they already exist at /usr/share/pioneer/.
  3. If creating a package the new package will be missing all of the .sgm files which will then vanish from the system when the package is upgraded.

My pioneer version (and OS):

OS: Slackware64-current
pioneer: 9fc04e0

@orbea
Copy link
Contributor Author

orbea commented Jun 21, 2020

@impaktor and @Web-eWorks Any news on this issue?

@impaktor
Copy link
Member

impaktor commented Jul 19, 2020

@orbea I think it's a feature that pioneer only generates the sgm files if they're missing, as next startup will be significantly faster if it can just load existing files.

But if you want some explicit "forced" re-generation option to re-generate of all of them, or a "clean" option or something, that will make life easier when packaging it, or am I missing something?

@orbea
Copy link
Contributor Author

orbea commented Jul 19, 2020

@impaktor This is a big problem with distro packages. It forces the user to uninstall pioneer before rebuilding it, otherwise the sgm files will never be generated and then removed when the new package is upgraded over the old one.

In my use case they should always be generated.

@impaktor
Copy link
Member

impaktor commented Jul 19, 2020

This sounds like a @fluffyfreak -problem.

@Web-eWorks
Copy link
Member

I have a half-started branch where I was attempting to rewrite the model loader and compiler that I will eventually come back to, if @fluffyfreak wants to take a crack at it, I'll push it to my fork next week 😄

@fluffyfreak
Copy link
Contributor

This sounds like a @fluffyfreak -problem.

Everything sounds like a fluffyfreak problem :D

@orbea
Copy link
Contributor Author

orbea commented Jul 23, 2020

Not to be pushy, but if there is anything that can be done to resolve or even work around this issue sooner rather than later it would be greatly appreciated. Its a bit of a blocker for me.

@fluffyfreak
Copy link
Contributor

@orbea would being able to pass in -f to force it to always rebuild the models be enough? Or would it have to be the inverse behaviour where it always rebuilds unless another flag is present?

@fluffyfreak
Copy link
Contributor

This has made me wish we use a cmd line argument parser like https://github.com/jarro2783/cxxopts
NB: I may use it in the future, for now some hacking will suffice

@orbea
Copy link
Contributor Author

orbea commented Jul 24, 2020

I'm not really set on which way it should be, if I can set a flag to always generate them with cmake that would be fine.

@fluffyfreak
Copy link
Contributor

Perfect because I think that setting a -f flag will be easier to code up. I made a start on it at lunchtime and will try to finish it asap (time permitting)

@Web-eWorks
Copy link
Member

@fluffyfreak please port from the hyg-database-parser PR - already have command line handling there

@fluffyfreak
Copy link
Contributor

Ok, short answer is that I'm not able to reproduce this issue.

@orbea some questions:

  • what command line are you using with the modelcompiler?
  • where do you run it from?
  • is it from the installed version of Pioneer or the checkout source folder?

@orbea
Copy link
Contributor Author

orbea commented Jul 25, 2020

@fluffyfreak I tried to reproduce it with the current master (f34619f).

However with the following script run from inside the pioneer git repo the .sgm files are never generated.

#!/bin/sh

set -eu

rm -rf build
mkdir -p build
cd build
cmake -DCMAKE_INSTALL_PREFIX=/tmp/install-pioneer/usr ..
make
make install DESTDIR=/tmp/destdir-pioneer

rm -rf /tmp/install-pioneer/
mv /tmp/destdir-pioneer/tmp/install-pioneer /tmp/install-pioneer

The pioneer install should be in /tmp/install-pioneer, but unlike my previous tests it never generates the .sgm files regardless if they are present.

@orbea
Copy link
Contributor Author

orbea commented Jul 25, 2020

Seems this regressed even more in the master.

Even my normal builds never generate .sgm files.

@orbea
Copy link
Contributor Author

orbea commented Jul 25, 2020

It entirely stopped generating the .sgm files for me with this commit.

97ad693c9ddf4eb9bc89362d4e6accfeaf203752 is the first bad commit
commit 97ad693c9ddf4eb9bc89362d4e6accfeaf203752
Author: Webster Sheets <webster@web-eworks.com>
Date:   Wed May 13 21:57:21 2020 -0400

    Add fmt::fmt for better formatting
    
    - Need to add to AUTHORS.txt, will do with string_view and argh

 CMakeLists.txt                                |   14 +-
 contrib/fmt/CMakeLists.txt                    |  332 +++
 contrib/fmt/LICENSE.rst                       |   27 +
 contrib/fmt/include/fmt/chrono.h              | 1119 ++++++++
 contrib/fmt/include/fmt/color.h               |  568 ++++
 contrib/fmt/include/fmt/compile.h             |  595 ++++
 contrib/fmt/include/fmt/core.h                | 1796 ++++++++++++
 contrib/fmt/include/fmt/format-inl.h          | 1403 ++++++++++
 contrib/fmt/include/fmt/format.h              | 3648 +++++++++++++++++++++++++
 contrib/fmt/include/fmt/locale.h              |   78 +
 contrib/fmt/include/fmt/os.h                  |  392 +++
 contrib/fmt/include/fmt/ostream.h             |  166 ++
 contrib/fmt/include/fmt/posix.h               |    2 +
 contrib/fmt/include/fmt/printf.h              |  726 +++++
 contrib/fmt/include/fmt/ranges.h              |  387 +++
 contrib/fmt/src/format.cc                     |  176 ++
 contrib/fmt/src/os.cc                         |  316 +++
 contrib/fmt/support/cmake/FindSetEnv.cmake    |    7 +
 contrib/fmt/support/cmake/cxx14.cmake         |   70 +
 contrib/fmt/support/cmake/fmt-config.cmake.in |    4 +
 contrib/fmt/support/cmake/fmt.pc.in           |   11 +
 21 files changed, 11834 insertions(+), 3 deletions(-)
 create mode 100644 contrib/fmt/CMakeLists.txt
 create mode 100644 contrib/fmt/LICENSE.rst
 create mode 100644 contrib/fmt/include/fmt/chrono.h
 create mode 100644 contrib/fmt/include/fmt/color.h
 create mode 100644 contrib/fmt/include/fmt/compile.h
 create mode 100644 contrib/fmt/include/fmt/core.h
 create mode 100644 contrib/fmt/include/fmt/format-inl.h
 create mode 100644 contrib/fmt/include/fmt/format.h
 create mode 100644 contrib/fmt/include/fmt/locale.h
 create mode 100644 contrib/fmt/include/fmt/os.h
 create mode 100644 contrib/fmt/include/fmt/ostream.h
 create mode 100644 contrib/fmt/include/fmt/posix.h
 create mode 100644 contrib/fmt/include/fmt/printf.h
 create mode 100644 contrib/fmt/include/fmt/ranges.h
 create mode 100644 contrib/fmt/src/format.cc
 create mode 100644 contrib/fmt/src/os.cc
 create mode 100644 contrib/fmt/support/cmake/FindSetEnv.cmake
 create mode 100644 contrib/fmt/support/cmake/cxx14.cmake
 create mode 100644 contrib/fmt/support/cmake/fmt-config.cmake.in
 create mode 100644 contrib/fmt/support/cmake/fmt.pc.in

97ad693

@Web-eWorks Is there any support for system versions of fmt?

@orbea
Copy link
Contributor Author

orbea commented Jul 26, 2020

The above commit made it an independent build target which I did not notice at first.

make build-data

@fluffyfreak That said I am not able to reproduce it with my synthetic tests, only with my actual Slackware package. Here is the basic sequence where this does occur.

  1. Pioneer and the .sgm files are built.
  2. Pioneer is installed to the DESTDIR (/tmp/SBo/package-pioneer).
  3. The DESTDIR is used to create a package.
  4. Pioneer is installed system wide using the package.
  5. Repeat steps 1-3.
  6. The new package lacks the .sgm files.
  7. The old install is removed and the new package is upgraded in place over it.
  8. The .sgm files are now completely gone since the new package did not contain them, only the old package does.

Does this help you understand? If the old package is uninstalled before building the new package this problem does not occur.

For reference I am using -DCMAKE_INSTALL_PREFIX=/usr and -DPIONEER_DATA_DIR=/usr/share/games/pioneer.

@fluffyfreak
Copy link
Contributor

Packages are so far outside of my experience that I have no idea.

@orbea
Copy link
Contributor Author

orbea commented Jan 21, 2021

A workaround is to build the modelcompiler first with an empty cmake environment, run the command manually and then proceed to rebuild pioneer normally.

mkdir build
cd build
cmake ..
make modelcompiler

cmake \
  -E env SDL_VIDEODRIVER=dummy \
  /path/to/pioneer/build/modelcompiler -b inplace
 
cmake <cmake arguments here> ..
make
make install DESTDIR=$PKG
```

@The-EG
Copy link
Contributor

The-EG commented Jan 21, 2021

This sounds suspiciously like it's related to the issue referenced in #5039 , where the modelcompiler will use the system data dir if it exists, and won't build any models because the system data dir only has the compiled models, etc...

edit: reading back through this, this sounds like this is the exact issue fixed in #5039

That should have been resolved by that change. Have you tried it since then without any work arounds?

@orbea
Copy link
Contributor Author

orbea commented Jan 21, 2021

@The-EG Yes, you are right. This is fixed in the git master, thanks for pointing that out!

@orbea orbea closed this as completed Jan 21, 2021
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

No branches or pull requests

5 participants