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

Allow using basedir as a root base directory for files and paths. #2303

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Oct 16, 2024

What does this PR do?

This adds some improvements to basedir so it can be used to specify a base directory for file, path and directory fields.

How does this PR change Premake's behavior?

Previously it only could be used for tokens, but now it can also affects paths, so depending on the order it's set, it might provide different results.

Anything else we should know?

Another option would be to go with a new srcdir, that also seems like a reasonable approach to take.

So feel free to provide some feedback if you have preferences how this should work.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

Closes #2129.

@tritao tritao changed the title Basedir improvements Allow using basedir as a root base directory for files and paths. Oct 16, 2024
@tritao tritao marked this pull request as ready for review October 16, 2024 21:25
@nickclark2016
Copy link
Member

This reads as an extension of existing behavior, rather than breaking the existing behavior. Would this be a correct interpretation of your goals?

@nickclark2016
Copy link
Member

This reads as an extension of existing behavior, rather than breaking the existing behavior. Would this be a correct interpretation of your goals?

@tritao Just pinging for response here

@tritao
Copy link
Contributor Author

tritao commented Nov 11, 2024

This reads as an extension of existing behavior, rather than breaking the existing behavior. Would this be a correct interpretation of your goals?

Yes, it's an extension of existing behavior. I think it should be safe to merge, my only concern is that basedir is already used for a niche use case (generating completely portable project files), but it's barely documented and there's like a single test for it. Since that is still available and works, it seems reasonable to use the same property for the functionality I introduced here, but I could also be reasoned to introduce a new distinct API.

@nickclark2016
Copy link
Member

We are still in API break territory since we haven't hit 5.0. It could be worth some investigation into projects using Premake to see if they're using the undocumented behavior (hopefully no Hyrum's Law yet).

@nickclark2016
Copy link
Member

The more I think about this, I think the better it is to get the @premake/full-team thoughts, since it would be a potentially breaking change for some.

@samsinsane
Copy link
Member

I don't think I'm really following what basedir currently does from what has been said, and the docs suggest it should do what the current PR implements - so I'm not sure what basedir is for, but if the functionality is different then maybe we should have a new API to avoid the inevitable complaints.

I haven't tested anything, but does files { "**.cpp" } give different results after each call to basedir? For example:

basedir "a"
files { "**.cpp" }
basedir "b"
files { "**.cpp" }

Is this the equivalent of files { "a/**.cpp", "b/**.cpp" }? If not, should it be? I didn't see a test for this, so should probably include that regardless of which way it works.

What do the changes in oven.lua do? I might be wrong, but I don't think it's required for the api.lua changes?

@nickclark2016
Copy link
Member

Personally, I'm fine with breaking the API here. At a minimum, it's a pretty minor break and we can put it in the release notes. Can you provide a minimum example of what would break?

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 this pull request may close these issues.

Allow specifying root directory for the project sources
3 participants