libretro / bsnes-mercury

Fork of bsnes with various performance improvements.
GNU General Public License v3.0
47 stars 39 forks source link

Use the standard 'rm -f' for the make clean target. #59

Closed orbea closed 6 years ago

orbea commented 6 years ago

See PR https://github.com/libretro/bsnes-libretro-cplusplus98/pull/18 for the reasoning behind this.

Doing this will also help reduce complexity in the buildbot script.

@Alcaro Would you mind taking a look at this PR and making sure its acceptable? Thanks. :)

Alcaro commented 6 years ago

That will fail if someone installs mingw-w64 without a msys shell or similar. I used that specific setup back when I was on Windows (though I'll admit I didn't compile any cores at all, except libretro-samples/test-advanced née minir-test-core).

I'm fine with having msys shell use rm, but I'm not sure if breaking that usecase is worth it.

Considering this PR claims 'see the other one' as the only reason, and that one claims only code cleanup, I'm gonna vote not worth it. If buildbot needs this, explain why exactly.

Since no other core tolerates such a setup, I'll accept breaking that usecase if it brings actual benefits. "Buildbot needs it because " counts as actual benefit, but "buildbot needs it because buildbot needs it" is not.

orbea commented 6 years ago

rm -f i used in most of the core Makefiles for the make clean targets? See mgba for example.

Due to the reason that this silences all make clean output the buildbot avoids the make clean target for only the bsnes (And higan cores).

https://github.com/libretro/libretro-super/blob/master/libretro-buildbot-recipe.sh#L654

It would not be hard to get rid of the extra bsnes make functions, but not being able to use make clean will just needlessly complicate that and add to the mess that already exists. :)

That said, I am not fond of breaking portability, do you know of an alternative that will not break anything?

orbea commented 6 years ago

Also for the record, I am willing to update the other bsnes cores with any better solution that can be reached. :)

orbea commented 6 years ago

Also are such setups supported by any libretro frontend? I have a hard time imagining the RetroArch configure scripts would work without a shell, how about other frontends?

Alcaro commented 6 years ago

RetroArch's ./configure won't even try on native Windows. But I'm not sure if it needs to be configured at all on Windows; last time I checked, it didn't. But that was a while ago, I don't know if it's still true.

Other than that, I am only aware of one front that doesn't prefer MSVC on Windows: minir. It, like bsnes, prefers mingw-w64 without msys.

Silencing output is a valid goal. I'm not aware of a way to make del shut up, but ol' devnull is close enough.

delete = del /F /Q $(subst /,\,$1) 2>nul

(hopefully works inside make, I only tried on native cmd. I don't have any dev tools on that machine, and don't want them.)

orbea commented 6 years ago

I must of not explained very well, the goal is to have it not silence output so the buildbot logs can include the output of make install. Currently at least here with GNU make make clean prints nothing.

Alcaro commented 6 years ago

If your goal is more verbosity, just remove the @s from the invocations.

Keep the -s, or make clean will fail if one of the dels fail, for example if the file didn't exist.

orbea commented 6 years ago

Given your valid concerns I will close this PR and proceed cleaning up the buildbot script without this convenience. That said I'll leave bsnes-cplus-plus as is for now since that is working well for the buildbot and anyone with your concern should really use bsnes-mercury instead. If anyone can think of a good alternative, let me know.

orbea commented 6 years ago

I didn't see your reply till after, that said its not a big issue and I will revisit this later.

Alcaro commented 6 years ago

I'll just delete those @s. @s are great for getting rid of walls of text, like RetroArch (try make V=1 if you want a demonstration), but it has zero utility for outputs that are already small, like make clean.

orbea commented 6 years ago

Thanks!