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

vvvvvv: init at 2.3-git-2020-11-22 #79068

Open
wants to merge 6 commits into
base: master
from
Open

vvvvvv: init at 2.3-git-2020-11-22 #79068

wants to merge 6 commits into from

Conversation

@anna328p
Copy link
Member

@anna328p anna328p commented Feb 2, 2020

Adds a package for VVVVVV, a platform game based on inverting gravity.

Motivation for this change

Adds the game VVVVVV to nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notes

Fixed version of #78319 which was broken after a bad merge into my nixpkgs fork.

@sikmir
Copy link
Member

@sikmir sikmir commented Feb 2, 2020

Could you please squash commits into one?

pkgs/games/vvvvvv/default.nix Outdated Show resolved Hide resolved
@nh2
Copy link
Contributor

@nh2 nh2 commented Feb 2, 2020

For reference, this is a replacement for the previous PR #78319.

CCing people who commented there so they don't miss out on notifications: @leo60228 @jakobrs

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 2, 2020

Should there not be separate pkgs.vvvvvvMakeAndPlay and pkgs.vvvvvvFull attributes?

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 2, 2020

Idea: If someone implements the ability to load data.zip from a custom location, the binary could be factored out into a separate derivation, which might be redistributable.

/nix/store/...-data.zip             definitely unfree
/nix/store/...-vvvvvv-full-bin-...  unfreeRedistributable
/nix/store/...-vvvvvv-full-...      unfree, I think

The last of the three derivations would simply be a wrapper script that calls vvvvvv-bin with the right assets.

#!/bin/sh
${vvvvvvBin}/bin/VVVVVV --assets ${dataZip}
@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 2, 2020

I can do this when I get home in about 30 minutes.

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 2, 2020

On the legal side of things:

@TerryCavanagh, are you OK with us redistributing the full-version binary without redistributing the assets?

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 2, 2020

diff --git a/desktop_version/src/FileSystemUtils.cpp b/desktop_version/src/FileSystemUtils.cpp
index 2a1d2df..a7db5ca 100644
--- a/desktop_version/src/FileSystemUtils.cpp
+++ b/desktop_version/src/FileSystemUtils.cpp
@@ -39,7 +39,7 @@ void PLATFORM_getOSDirectory(char* output);
 void PLATFORM_migrateSaveData(char* output);
 void PLATFORM_copyFile(const char *oldLocation, const char *newLocation);
 
-int FILESYSTEM_init(char *argvZero)
+int FILESYSTEM_init(char *argvZero, char *dataPath)
 {
 	char output[MAX_PATH];
 	int mkdirResult;
@@ -79,9 +79,7 @@ int FILESYSTEM_init(char *argvZero)
 	}
 
 	/* Mount the stock content last */
-	strcpy(output, PHYSFS_getBaseDir());
-	strcat(output, "data.zip");
-	if (!PHYSFS_mount(output, NULL, 1))
+	if (!PHYSFS_mount(dataPath, NULL, 1))
 	{
 		puts("Error: data.zip missing!");
 		puts("You do not have data.zip!");
diff --git a/desktop_version/src/FileSystemUtils.h b/desktop_version/src/FileSystemUtils.h
index d3c551e..404896b 100644
--- a/desktop_version/src/FileSystemUtils.h
+++ b/desktop_version/src/FileSystemUtils.h
@@ -6,7 +6,7 @@
 
 #include "tinyxml.h"
 
-int FILESYSTEM_init(char *argvZero);
+int FILESYSTEM_init(char *argvZero, char *dataPath);
 void FILESYSTEM_deinit();
 
 char *FILESYSTEM_getUserSaveDirectory();
diff --git a/desktop_version/src/main.cpp b/desktop_version/src/main.cpp
index 4dba0fa..76c79f3 100644
--- a/desktop_version/src/main.cpp
+++ b/desktop_version/src/main.cpp
@@ -44,7 +44,7 @@ entityclass obj;
 
 int main(int argc, char *argv[])
 {
-    if(!FILESYSTEM_init(argv[0]))
+    if(!FILESYSTEM_init(argv[0], argv[1]))
     {
         return 1;
     }
@@ -55,9 +55,9 @@ int main(int argc, char *argv[])
         SDL_INIT_GAMECONTROLLER
     );
 
-    if (argc > 2 && strcmp(argv[1], "-renderer") == 0)
+    if (argc > 3 && strcmp(argv[2], "-renderer") == 0)
     {
-        SDL_SetHintWithPriority(SDL_HINT_RENDER_DRIVER, argv[2], SDL_HINT_OVERRIDE);
+        SDL_SetHintWithPriority(SDL_HINT_RENDER_DRIVER, argv[3], SDL_HINT_OVERRIDE);
     }
 
     NETWORK_init();
@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 2, 2020

Remember that you’ll probably want to make sure that that can be merged into upstream, as otherwise this package will have to be marked as modified.

From the license:

Altered source/binary versions must be plainly marked as such, and must not be misrepresented as being the original software.

Edit: What I mean is that it is important that the syntax is compatible with the old invocation syntax, as otherwise it probably won’t be merged into VVVVVV itself.

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 2, 2020

@TerryCavanagh
Copy link

@TerryCavanagh TerryCavanagh commented Feb 2, 2020

@jakobrs Yep, it's fine to distribute the full version binary without the data.zip assets included.

Copy link
Contributor

@jonringer jonringer left a comment

please squash the commits as well please:

git reset HEAD^^^
git add pkgs/
git commit --amend --no-edit
git push .. .. --forcce
@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 2, 2020

The PR adding -assets was just merged.

@anna328p anna328p force-pushed the anna328p:vvvvvv branch from 3e128c7 to d121fd2 Feb 2, 2020
@anna328p
Copy link
Member Author

@anna328p anna328p commented Feb 2, 2020

Should there not be separate pkgs.vvvvvvMakeAndPlay and pkgs.vvvvvvFull attributes?

can't that be done in all-packages.nix?

vvvvvvFull = callPackage ../games/vvvvvv { fullGame = true };
@anna328p
Copy link
Member Author

@anna328p anna328p commented Feb 2, 2020

here's the problem:

the officially distributed data.zip is the same as the Make and Play Edition data.zip, they have the same hash. the only difference between these two versions of the game is the flags used to build the game. if -DMAKEANDPLAY is set then the game runs in the Make and Play mode.

if the binary version is distributed with the make and play flag disabled, you can use the zip from the make and play version to play the game. therefore, if you have the full game, you must supply the zip yourself and the binary is built locally as it is not allowed to be redistributed. if you do not have the full game, you get the make and play binary which can be redistributed with the make and play zip (again, this is the same as the standard zip)

@anna328p
Copy link
Member Author

@anna328p anna328p commented Feb 2, 2020

Could you please squash commits into one?

Doesn't Github have an option to automatically do that when merging a pull request?

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 3, 2020

Doesn't Github have an option to automatically do that when merging a pull request?

Yes, but it doesn't follow the commit naming scheme used for nixpkgs.

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 3, 2020

That problem (with the Make and Play edition using the same zip file) already exists:

nix-env -iA nixpkgs.vvvvvv      # Fetches the data.zip for you
nix-env -iA nixpkgs.vvvvvvFull  # Uses the data.zip fetched in the previous step

The obvious solution is either not to fetch the data.zip in either case, or somehow change the hash of the data.zip file.

@anna328p
Copy link
Member Author

@anna328p anna328p commented Feb 3, 2020

That problem (with the Make and Play edition using the same zip file) already exists:

nix-env -iA nixpkgs.vvvvvv      # Fetches the data.zip for you
nix-env -iA nixpkgs.vvvvvvFull  # Uses the data.zip fetched in the previous step

The obvious solution is either not to fetch the data.zip in either case, or somehow change the hash of the data.zip file.

This problem already exists outside nixpkgs though; anyone can take the publicly available source and the publicly available make and play edition data.zip, and compile a complete version of the game.

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 3, 2020

Maybe have fetchurl rename the Make & Play data.zip so they have different hashes?

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 3, 2020

We could also do postFetch = "echo nixpkgs | zip -z $out";.

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 3, 2020

Yep, this seems to work:

let
  dataZip = if fullGame then requireFile {                                                                                                   
    # the data file for the full game                                                                                                        
    name = "data.zip";                                                                                                                       
    sha256 = "1q2pzscrglmwfgdl8yj300wymwskh51iq66l4xcd0qk0q3g3rbkg";                                                                         
    message = ''                                                                                                                             
      In order to install VVVVVV, you must first download the game's                                                                         
      data file (data.zip) as it is not released freely.                                                                                     
      Once you have downloaded the file, place it in your current                                                                            
      directory, use the following command and re-run the installation:                                                                      
      nix-prefetch-url file://\$PWD/data.zip                                                                                                 
    '';                                                                                                                                      
  } else fetchurl {                                                                                                                          
    # the data file for the free Make and Play edition                                                                                       
    url = https://thelettervsixtim.es/makeandplay/data.zip;                                                                                  
    sha256 = "0vnb4zyzp8jpayxrgs6mw98wxpcpqdcfx04j1g5s0hm378xwmxm6";                                                                         
    postFetch = "echo nixpkgs | ${zip}/bin/zip -z $out";                                                                                     
  };                                                                                                                                         
@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 3, 2020

@TerryCavanagh The license only applies to the binaries, right? That would make this hack of adding a zip file comment to the Make & Play data.zip so it has a different hash allowed. That's necessary because Nix treats files with the same name and hash as being the same file, so there's otherwise no way to distribute the Make & Play data.zip without also distributing the full game's data.zip. As a demonstration of this problem:

nix-env -iA nixos.vvvvvvFull # errors, no data.zip
nix-env -iA nixos.vvvvvv # downloads binaries and data.zip
nix-env -iA nixos.vvvvvvFull # succeeds, downloads binaries and uses data.zip already on system

Adding a zip file comment while downloading data.zip would cause it to have a different hash, solving this problem. This clause is worrying me:

Altered source/binary versions must be plainly marked as such, and must not be misrepresented as being the original software.

@anna328p
Copy link
Member Author

@anna328p anna328p commented Feb 3, 2020

The derivation now renames the Make and Play data file to mapdata.zip which has a different store path, and creates a wrapper that launches VVVVVV with the path to the correct file.

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 8, 2020

Patches to factor vvvvvv-bin out of vvvvvv and add full version to all-packages.nix: https://gist.github.com/jakobrs/61389d04fd49d73a53418eb6b9aaf969

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 8, 2020

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 8, 2020

Could you make a build with debugging symbols and get a backtrace with bt? Just knowing the function where it crashed, especially when it's in the standard library, isn't very helpful...

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 8, 2020

Here is the backtrace when compiled with dontStrip.

#0  0x00000000005947a4 in split(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) ()
#1  0x0000000000594cf7 in split(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char) ()
#2  0x000000000058e037 in towerclass::fillcontents(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) ()
#3  0x000000000058e204 in towerclass::loadmap() ()
#4  0x000000000059250a in towerclass::towerclass() ()
#5  0x00000000004cf160 in mapclass::mapclass() ()
#6  0x00000000004554e7 in _GLOBAL__sub_I_main.cpp ()
#7  0x00000000005c5ebd in __libc_csu_init ()
#8  0x00007ffff76ffb1d in __libc_start_main () from /nix/store/aag9d1y4wcddzzrpfmfp9lcmc7skd7jk-glibc-2.27/lib/libc.so.6
#9  0x00000000004555ba in _start () at ../sysdeps/x86_64/start.S:120

Does that help?

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 8, 2020

It's more helpful, but that doesn't seem to include line numbers from DWARF. I'm not actually sure how to get those with Nix...

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 8, 2020

Try prepending pkgs.enableDebugging to the package (i.e. nix-shell -I nixpkgs=. -p 'enableDebugging vvvvvv').

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 8, 2020

I've made https://gist.github.com/15533e667703b8fbd392a9cd9757f758 so that vvvvvv-bin can be overridden.

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 8, 2020

I’m not sure, but I think you’re missing fullGame = true; to vvvvvv-full.

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 8, 2020

vvvvvv inherits fullGame from the vvvvvv-bin it's called with in my patch. I've confirmed that PR fixes the segfault.

@jakobrs
Copy link
Contributor

@jakobrs jakobrs commented Feb 9, 2020

I've confirmed that TerryCavanagh/VVVVVV#155 does fix the segfault.

@anna328p
Copy link
Member Author

@anna328p anna328p commented Feb 9, 2020

what is the point of the vvvvvv-bin package?

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Feb 10, 2020

vvvvvv-bin is the binaries. vvvvvv is a wrapper to add data.zip. This is required so that the full version can legally be built on Hydra.

jakobrs added a commit to jakobrs/nixpkgs that referenced this pull request Feb 16, 2020
Adds a package for VVVVVV, a platform game based on inverting gravity.

Relevant:

- NixOS#77519
- NixOS#78319
- NixOS#79068
- TerryCavanagh/VVVVVV#139
- TerryCavanagh/VVVVVV#153
- TerryCavanagh/VVVVVV#155
- TerryCavanagh/VVVVVV#156

- git log at eb3527f

Co-authored-by: jakobrs <jakobrs100@gmail.com>
Co-authored-by: leo60228 <iakornfeld@gmail.com>
jakobrs added a commit to jakobrs/nixpkgs that referenced this pull request Feb 16, 2020
Adds a package for VVVVVV, a platform game based on inverting gravity.

Relevant:

- NixOS#77519
- NixOS#78319
- NixOS#79068
- TerryCavanagh/VVVVVV#139
- TerryCavanagh/VVVVVV#153
- TerryCavanagh/VVVVVV#155
- TerryCavanagh/VVVVVV#156

- git log at eb3527f

Co-authored-by: jakobrs <jakobrs100@gmail.com>
Co-authored-by: leo60228 <iakornfeld@gmail.com>
jakobrs added a commit to jakobrs/nixpkgs that referenced this pull request Mar 13, 2020
Adds a package for VVVVVV, a platform game based on inverting gravity.

Relevant:

- NixOS#77519
- NixOS#78319
- NixOS#79068
- TerryCavanagh/VVVVVV#139
- TerryCavanagh/VVVVVV#153
- TerryCavanagh/VVVVVV#155
- TerryCavanagh/VVVVVV#156

- git log at eb3527f

Co-authored-by: jakobrs <jakobrs100@gmail.com>
Co-authored-by: leo60228 <iakornfeld@gmail.com>
jakobrs added a commit to jakobrs/nixpkgs that referenced this pull request Mar 13, 2020
Adds a package for VVVVVV, a platform game based on inverting gravity.

Relevant:

- NixOS#77519
- NixOS#78319
- NixOS#79068
- TerryCavanagh/VVVVVV#139
- TerryCavanagh/VVVVVV#153
- TerryCavanagh/VVVVVV#155
- TerryCavanagh/VVVVVV#156

- git log at eb3527f

Co-authored-by: jakobrs <jakobrs100@gmail.com>
Co-authored-by: leo60228 <iakornfeld@gmail.com>
@MetaDark
Copy link
Contributor

@MetaDark MetaDark commented Jul 20, 2020

@dkudriavtsev The repo for VVVVVV now has a release tag. It's the initial commit pushed to the repo, so it's an older version, but it's probably better to use than an arbitrary revision.

@anna328p
Copy link
Member Author

@anna328p anna328p commented Jul 20, 2020

@dkudriavtsev The repo for VVVVVV now has a release tag. It's the initial commit pushed to the repo, so it's an older version, but it's probably better to use than an arbitrary revision.

2.3 is about to release. I'll wait until then.

@anna328p anna328p force-pushed the anna328p:vvvvvv branch from 262ca79 to 871c831 Jul 29, 2020
@anna328p
Copy link
Member Author

@anna328p anna328p commented Jul 29, 2020

Actually, v2.3's release date got bumped to October 1st.

The current master branch of the repository builds cleanly and has some substantial feature improvements over version 2.2. I'll bump the package to the latest revision and make the changes required to separate the full game and Make and Play Mode.

anna328p and others added 6 commits Jan 22, 2020
Adds a package for VVVVVV, a platform game based on inverting gravity.
- changed version to be a date instead of a fragment of the commit id
- quoted homepage url
- if user does not own the full game, build the redistributable "make
  and play version" instead
- using sourceRoot instead of manually changing directory
@anna328p anna328p force-pushed the anna328p:vvvvvv branch from 871c831 to b9f9877 Jul 29, 2020
homepage = "https://thelettervsixtim.es";
license = if fullGame then licenses.unfree else licenses.unfreeRedistributable;
maintainers = [ maintainers.dkudriavtsev ];
platforms = [ platforms.linux platforms.darwin ];

This comment has been minimized.

@jonringer

jonringer Aug 5, 2020
Contributor

this makes [[string]] we want [string]

Suggested change
platforms = [ platforms.linux platforms.darwin ];
platforms = platforms.linux ++ platforms.darwin;
SDL2 SDL2_mixer
] + stdenv.lib.optionalPlatform stdenv.lib.platforms.darwin [ Foundation ];

sourceRoot = "source/desktop_version";

This comment has been minimized.

@jonringer

jonringer Aug 5, 2020
Contributor

Suggested change
sourceRoot = "source/desktop_version";
sourceRoot = "source/desktop_version";
nativeBuildInputs = [ cmake ninja ];
buildInputs = [
SDL2 SDL2_mixer
] + stdenv.lib.optionalPlatform stdenv.lib.platforms.darwin [ Foundation ];

This comment has been minimized.

@jonringer

jonringer Aug 5, 2020
Contributor

optionalPlatform doesn't exist, i think you want

Suggested change
] + stdenv.lib.optionalPlatform stdenv.lib.platforms.darwin [ Foundation ];
] + stdenv.lib.optionals stdenv.hostPlatform.isDarwin [ Foundation ];
};

# if the user does not own the full game, build the Make and Play edition
flags = if fullGame then [] else [ "-DMAKEANDPLAY" ];

This comment has been minimized.

@jonringer

jonringer Aug 5, 2020
Contributor

Suggested change
flags = if fullGame then [] else [ "-DMAKEANDPLAY" ];
flags = lib.optionals fullGame [ "-DMAKEANDPLAY" ];
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.