UltraStar-Deluxe / USDX

The free and open source karaoke singing game UltraStar Deluxe, inspired by Sony SingStar™
https://usdx.eu
GNU General Public License v2.0
832 stars 160 forks source link

Following Linux build instructions pollutes git #636

Closed barbeque-squared closed 1 year ago

barbeque-squared commented 1 year ago

When following the build instructions as listed in the README, some files get modified/typechanged. This makes it easier for them to be accidentally committed. Probably the main causes ./autogen.sh and ./game/ultrastardx

Actual behaviour

$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    typechange: dists/autogen/config.guess
    typechange: dists/autogen/config.sub
    typechange: dists/autogen/install-sh
    modified:   game/config.ini
    modified:   src/media/UAudioInput_Portaudio.pas

Untracked files:
  (use "git add <file>..." to include in what will be committed)
    .idea/

no changes added to commit (use "git add" and/or "git commit -a")

Expected behaviour

If these are generated files, they shouldn't be in git in the first place, and secondly should get ignored by the .gitignore.

Maybe config.ini is included because it pre-setups some microphone (for end users)? But I'm pretty sure this only applies to portable versions (aka Windows) so I'm not sure what value it adds for anyone else.

Steps to reproduce

  1. Clone the repo and cd to the directory
  2. ./autogen.sh (this probably does something in the dists/autogen folder)
  3. make; ./game/ultrastardx (the latter will most likely change config.ini)

Details

Actually, the .gitignore already ignores /game/config.ini so it probably really shouldn't be there. The .idea folder is probably analog to the lazarus project files, idk what other editors are out there but it's fairly common to ignore it. The .idea folder is probably under the same

basisbit commented 1 year ago

I mostly run Lazarus on Windows, thus I never saw any of those files. Just adding them to .gitignore on demand sounds like a good fix.

basisbit commented 1 year ago

the /game/config.ini was added on purpose into the repository to define the "default" which gets also exported when a game version shipped. The lazarus project file is added to the repository on purpose so that the default settings for new developers already fit and so that LazarusIDE uses can just quickly start developing. They were added to the gitignore, so that no accidental changes get committed. This was on purpose.

s09bQ5 commented 1 year ago

I agree that the three files in dists/autogen/ should not be part of the git repository. For some strange reason the autoreconf 2.69 I had until a few minutes ago did not touch these files when running autogen.sh. The current version 2.71 does.

game/config.ini is not modified during build. It is deleted by make distclean and overwritten as soon as you start game/ultrastardx and assign your microphone.

src/media/UAudioInput_Portaudio.pas is never modified automatically.

barbeque-squared commented 1 year ago

Figured that config.ini would be the odd one. I do know that if the entire file is missing, the game falls back to built-in defaults. But I'm fine with postponing looking into that one, since it's the least annoying one anyway.

I'll PR the editor folder later (I tried Lazarus but didn't like it at all)

Thanks for the comment on the autogen files, I'll see about getting a PR on those as well.

The Portaudio one shouldn't have been in the listing, I copied the output while working on another PR but forgot to remove that line after pasting it here.

barbeque-squared commented 1 year ago

I've verified the autogen stuff being unnecessary by cloning the repo freshly, manually deleting those files, and then just doing the regular build process. Builds (and runs) just fine, so I've made some PR's. If nobody beats me to it, I'll merge them whenever AppVeyor is done.

barbeque-squared commented 1 year ago

Associated PR's are merged so this can also be closed