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

Windows - gmake2 case sensitivity #1241

Closed
kaldap opened this issue Feb 7, 2019 · 13 comments · Fixed by #1332
Closed

Windows - gmake2 case sensitivity #1241

kaldap opened this issue Feb 7, 2019 · 13 comments · Fixed by #1332

Comments

@kaldap
Copy link

kaldap commented Feb 7, 2019

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 target obj/Debug/Rtc.o'

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

@samsinsane
Copy link
Member

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
Copy link
Author

kaldap commented Feb 8, 2019

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
Copy link
Contributor

tritao commented Feb 8, 2019

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
Copy link

Neonit commented Aug 28, 2019

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
Copy link
Member

starkos commented Aug 28, 2019

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
Copy link
Author

kaldap commented Aug 29, 2019

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

@Neonit
Copy link

Neonit commented Aug 30, 2019

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
Copy link
Member

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
Copy link

Neonit commented Aug 31, 2019

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
Copy link
Member

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
Copy link
Member

starkos commented Sep 1, 2019

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
Copy link
Contributor

ratzlaff commented Sep 5, 2019

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
Copy link
Contributor

ratzlaff commented Sep 9, 2019

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.

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

Successfully merging a pull request may close this issue.

6 participants