premake / premake-core

Premake
https://premake.github.io/
BSD 3-Clause "New" or "Revised" License
3.22k stars 620 forks source link

Windows - gmake2 case sensitivity #1241

Closed kaldap closed 5 years ago

kaldap commented 5 years ago

I am using latest Premake 5.0.0-dev on Windows with gmake2 (i am using the arm-none-eabi-* toolchain) builder.

There is a problem with two files of same name but unmatched cases, f.e.: a/Rtc.cpp and b/rtc.c. It is resolved to Rtc.o and rtc.o which is correct on case-sensitive platforms like Linux, but on Windows version of make it results to overriding one file with another and building only the latest with the following warning message:

WAS_Tag.make:1094: warning: overriding commands for target obj/Debug/Rtc.o' WAS_Tag.make:863: warning: ignoring old commands for targetobj/Debug/Rtc.o'

I have come with this quick'n'dirty fix, but it probably is not the best solution.

samsinsane commented 5 years ago

I'm not really familiar with the code that you changed, do all object filenames come out lowercase now or does it only impact the comparisons? If the filenames aren't changed, then I don't really see any issue with the change, but it'll need unit tests to ensure that it continues to work.

kaldap commented 5 years ago

It was meant more like a proof-of-concept thing as it did broke VS project generating. What I have been talking about ("not the best solution") is to introduce some more elegant and systematic solution. I've decided to modify the gmake2_cpp module to generate whole directory tree structure in the object directory which mirrors the original source tree structure and avoids the conflicts in the first place. It seems that this solution is used often with Makefiles. It may require some further changes, but the current version I am using now can be found here.

tritao commented 5 years ago

By the way, this is also one of the changes that GENie fork does: https://github.com/bkaradzic/GENie#changelog-since-fork

Allow source files in the same project to have the same name. Added SingleOutputDir flag to use single output directory (original behaviour).

Neonit commented 5 years ago

This issue cost me quite some time.

Please fix. Why don't you just add a random 8 character hex code in front of each file or encode the full path into the name?

Example File Tree

foo
  bar.cpp
foo-bar.cpp
foo--bar.cpp
Bar.cpp

Example 1

0a64be8f-bar.o
f7ab387e-Bar.o
48fe920a-foo-bar.o
...

Example 2

foo--bar.o // <- foo-bar.cpp (duplicate present hyphens in name to prevent clashes)
foo----bar.o // <- foo--bar.cpp
foo-bar.o // <- foo/bar.cpp
Bar.o // <- Bar.cpp
starkos commented 5 years ago

Why don't you just add a random 8 character hex code

We strive to make Premake's output deterministic (i.e. running Premake twice on the same input gives the same output). This is important for dev environments where the generated files need to be checked into source control to share with other (non-Premake using) groups.

kaldap commented 5 years ago

I am sure, he did not mean "completely random", but something like "randomly chosen hash algorithm". 😉

Neonit commented 5 years ago

Didn't specify that, but didn't think of deterministic output names either. But it makes sense to me to keep it deterministic now that you mentioned it. You could also just count up, given a deterministic order.

But what about the other suggestion to encode the path?

I'm working with @kaldap's dirty fix for now. (I'm using gmake2 only anyway.) Couldn't get their newer fix to work with the current version of Premake right away and didn't want to invest time fixing.

samsinsane commented 5 years ago

But what about the other suggestion to encode the path?

The function that @kaldap modified already handles filename clashes, in fact it does one of your suggestions:

You could also just count up, given a deterministic order.

It just assumes a case sensitive filesystem, which is what the "dirty fix" changes. Some tests and a PR is all that's really required for this to move forward.

Neonit commented 5 years ago

Well, about their quick, dirty fix they wrote:

[...] it did broke VS project generating.

So I assume it's not a candidate for solving this issue.

The other solution:

[...] the current version I am using now can be found here.

I have tried to merge it with the HEAD and it didn't work out. So I'd say there is more to do than some tests and a PR.

samsinsane commented 5 years ago

As far as I'm concerned that "dirty fix" is the preferred fix, it's simple and doesn't rewrite an otherwise working system.

starkos commented 5 years ago

Agree with @samsinsane…looks to me that all this needs is to be submitted as a PR with a unit test or two to cover the use case.

It was meant more like a proof-of-concept thing as it did broke VS project generating.

Can you share some details? Don't see why that would be the case, from this particular fix.

ratzlaff commented 5 years ago

Oh hey, this building upon something I contributed. For reference (where to put tests mostly) of the origin of oven.uniqueSequence: PR #1191 May be able to work on this next week if noone gets to it first.

ratzlaff commented 5 years ago

I think I fixed this: https://github.com/ratzlaff/premake-core/tree/fix_case_insensitive_collisions Only has gmake2 tests currently, will add more tomorrow before making a PR.