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

filters break with spurious whitespace #2420

Closed
1 of 13 tasks
crazydef opened this issue Jan 15, 2025 · 7 comments · Fixed by #2440
Closed
1 of 13 tasks

filters break with spurious whitespace #2420

crazydef opened this issue Jan 15, 2025 · 7 comments · Fixed by #2440
Assignees
Labels

Comments

@crazydef
Copy link

What seems to be the problem?
Filters break when the filter string contains additional whitespace.

For example, the following works:

filter { "configurations:Release" }
filter { "configurations:not Release" }
filter {}

while the following does not:

filter { "configurations: Release" }
filter { "configurations: not Release" }
filter {}

(Note the spaces after the colon.)

What did you expect to happen?
I expected whitespace to be ignored where possible.

What have you tried so far?
I removed the whitespace.

  • Visual Studio 2022 (vs2022)
  • Visual Studio 2019 (vs2019)
  • Visual Studio 2017 (vs2017)
  • Visual Studio 2015 (vs2015)
  • Visual Studio 2012 (vs2012)
  • Visual Studio 2010 (vs2010)
  • Visual Studio 2008 (vs2008)
  • Visual Studio 2005 (vs2005)
  • GNU Makefile (gmake)
  • GNU Makefile Legacy (gmakelegacy)
  • XCode (xcode)
  • Codelite
  • Other (Please list below)

What version of Premake are you using?
5.0.0-beta2

@crazydef crazydef added the bug label Jan 15, 2025
@crazydef
Copy link
Author

If this isn't fixed, it should at least be documented.

@nickclark2016
Copy link
Member

Let me get some tests written to reproduce this, then I'll try to take a look.

@nickclark2016 nickclark2016 self-assigned this Mar 4, 2025
@nickclark2016
Copy link
Member

To keep you in the loop, what it appears is happening is that it's searching for a configuration named Release and not Release, which are completely legal as currently specified. While for configurations, this may not be what is desired, we use the same mechanism for things like file filters, and most file systems allow for leading or trailing spaces on the file name (though it's likely not best practice for your filesystem). I am currently leaning towards a document the behavior explicitly and no code changes here. There are too many edge cases for this type of sanitization, leaving open a pretty large surface area for bugs.

@crazydef
Copy link
Author

crazydef commented Mar 4, 2025

Thanks for the update.

I would say documentation is the bare minimum here. It's an easy enough problem to work around - once you know what the problem is.

Would it be valid/make sense to print an error message if a filter refers to something it can't find?

@nickclark2016
Copy link
Member

I definitely agree that a documentation update for an immediate resolution is needed, and a future internal work task for the team to investigate input sanitization and normalization.

As for error messages, I don't think this would be appropriate. It's perfectly legal for it to match nothing, and that could be the expected behavior depending on the build set up. Perhaps a verbose log at the most would be useful here.

@crazydef
Copy link
Author

crazydef commented Mar 4, 2025

Yeah, I guess it's quite feasible to have filterable values only available on some platforms, etc. I do that myself, thinking about it.

Is it true to say not Release is a valid configuration? I can see that not Release (with two spaces) could be, but surely you have control over the not, no? (What happens if someone names something with a prefix of not ? Do you generate not not as the inverse? Or is not a reserved word at least?)

@nickclark2016
Copy link
Member

and a future internal work task for the team to investigate input sanitization and normalization.

As it currently stands, yes not Release would be a valid configuration name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants