ubisoft / Sharpmake

Sharpmake is an open-source C#-based solution for generating project definition files, such as Visual Studio projects and solutions, GNU makefiles, Xcode projects, etc.
Apache License 2.0
945 stars 171 forks source link

Fixed issue when two paths resolved to the same resolved path #226

Closed firesgc closed 1 year ago

firesgc commented 2 years ago

Hello,

I have the problem that two paths are resolved into the same full path. This case is not handled and leads to an assert (Strings.cs Ln 542). In my case, "[project.SourceRootPath]/Interface" and "[project.SharpmakeCsPath]/Interface" were resolved into the same full path.

To be honest, it's probably a bad design of my Sharpmake project (I use a base class for the project that adds some default include paths for all projects), but finding the problem was a bit difficult, and I think in the end it's a bug that two different paths can't resolve to the same full path.

Cheers

belkiss commented 2 years ago

Heya! Indeed, that limitation is a little weird, so I'd be open to merge this in, provided that it comes with a small unit-test to validate the behavior :) Thanks!

firesgc commented 2 years ago

I added a test case and found another place where path duplicates are not checked (even though this is already done in PathUtils.ResolvePathAndFixCase()).

belkiss commented 2 years ago

Thanks for adding the test!

Note that sthg that needs testing as well is what happens when 2 items resolving to the same path are added in an OrderableStrings, but with 2 different weights. It should throw an error

firesgc commented 2 years ago

Oh, I'm not aware of any kind of weighting in the resolver, and to be honest, I can't find it in the source code either. Can you point me to a place where you use it?

belkiss commented 2 years ago

Ofc, this is the Add method taking a weight:

https://github.com/ubisoft/Sharpmake/blob/d605a617c29deb4f007791877f2fbd129b4867de/Sharpmake/Strings.cs#L256

And it's used like this for instance:

conf.IncludePaths.Add("[pathC]"); // default is 0
conf.IncludePaths.Add("[pathB]", 1);
conf.IncludePaths.Add("[pathA]", 2);

Sharpmake will sort the entries by weight, and the ones with the same weight alphabetically. So in the above example, we'll get pathC, pathB, pathA

This is done when you want to enforce some includes ordering (or lib paths, etc).

firesgc commented 2 years ago

It's getting ugly :-D I need to add a special case for the OrderableStrings to copy the order number. I hope this is ok. I also updated the test case.

I hope I'm getting closer :-D

firesgc commented 2 years ago

Good morning!

I have a question regarding the OrderableStrings class: The SetOrRemoveAtIndex function completely ignores the orderNumber. Is this intentional and if so, why is it ok to ignore them in this case but not in the resolver?

Thank you!

belkiss commented 2 years ago

Heya! Hum indeed sounds like a bug/oversight.