Skip to content

feat: Add new field build.requires #992

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

Merged
merged 3 commits into from
Feb 21, 2025
Merged

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Feb 17, 2025

TODO:

  • Define a new field to be inserted in build-system.requires
  • Document new feature
  • Add support for {root:uri}
    • should we copy hatchling's implementation or resolve it only if hatchling is added to dependency?
  • Do we add a special case for sdist building or document that the user should define it? Postponed because it would require a more involved process to validate.

@henryiii
Copy link
Collaborator

henryiii commented Feb 17, 2025

Do we need {root:uri} if we have overrides? I thought that was a way to control the dependencies based on how it was being built?

And for sdist-building, I think we already have an override selector for that case?

@LecrisUT
Copy link
Collaborator Author

And for sdist-building, I think we already have an override selector for that case?

Yes. The question for this PR, is if we should provide a sane default if the user did not provide one, or we leave it completely to the user

Do we need {root:uri} if we have overrides? I thought that was a way to control the dependencies based on how it was being built?

Kind-of. The issue with file: is that relative paths are expanded relative to where pip install is called, so you cannot make a consistent pip install for the users without a more specialized handling. Although, I guess the user could do

[[tool.scikit-build.overrides]]
if.env.BUILD_ROOT = true
build.build-requires = ["foo @ file:${BUILD_ROOT}/foo"]

and make sure BUILD_ROOT=$(pwd). Not as convenient as having a {root:uri} setup and relying on default CI environment variable, but maybe it's a better initial format for it.

@henryiii
Copy link
Collaborator

I guess my question is, in what cases would this be incorrect?

[[tool.scikit-build.overrides]]
if.from-sdist = false
build.build-requires = ["foo @ ./foo"]

[[tool.scikit-build.overrides]]
if.from-sdist = true
build.build-requires = ["foo"]

@LecrisUT
Copy link
Collaborator Author

I guess my question is, in what cases would this be incorrect?

[[tool.scikit-build.overrides]]
if.from-sdist = false
build.build-requires = ["foo @ ./foo"]

[[tool.scikit-build.overrides]]
if.from-sdist = true
build.build-requires = ["foo"]
$ pip install .
(this works fine assuming there is /foo)
$ cd bar && pip install ../
(this breaks because it is checking for /bar/foo)

(don't we need to specify @ file:./foo instead of just @ ./foo. Don't remember if I checked if there was a difference there)

@henryiii
Copy link
Collaborator

Okay, since we already use Python formatting in build-dir, we could do it here too. And implementing {root:uri} should be easy. Not so sure about {root:parent:uri}. Hmm, it seems __format__ passes the colon in. Okay, guess we could do it:

class DispPath(Path):
    def __format__(self, fmt):
        command, _, rest = fmt.partition(":")
        if command == "parent":
            return self.parent.__format__(rest)
        if command == "uri" and rest == "":
            return self.as_uri()
        if command == "" and rest == "":
            return str(self)
        raise ...

@henryiii henryiii enabled auto-merge (squash) February 21, 2025 15:50
@henryiii henryiii merged commit 76cbc06 into scikit-build:main Feb 21, 2025
60 checks passed
@LecrisUT LecrisUT added this to the v0.11.0 milestone Feb 21, 2025
@LecrisUT LecrisUT linked an issue Feb 21, 2025 that may be closed by this pull request
henryiii added a commit that referenced this pull request Feb 24, 2025
…roject.toml` (#998)

Related to #992 formatter, I think it would be good to have a common
place where to define all the format variables. Not sure on the
implementation though if it should be a `TypedDict`, simple `Mapping`
wrapper or anything else.

---------

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: Henry Schreiner <[email protected]>
@LecrisUT LecrisUT changed the title feat: Add new field build.build-requires feat: Add new field build.requires Feb 27, 2025
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.

Support {root:uri}-like workflow in build-system.requires
2 participants