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

Switch to using build (instead of pep517) #233

Merged
merged 6 commits into from
Jan 3, 2021
Merged

Switch to using build (instead of pep517) #233

merged 6 commits into from
Jan 3, 2021

Conversation

nbraud
Copy link
Collaborator

@nbraud nbraud commented Dec 28, 2020

No description provided.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 28, 2020

@duckinator I ran the tests, and it builds one of my local projects without issue.
I also built bork as a zipapp (using itself, of course) and used the zipapp to rebuild bork and my project.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 28, 2020

I'm confused why the linting fails in the Zippapp bootstrap (and only then) ; I don't even see where does the import-order linter come from, let alone whether it is erroneously treating build as if it was part of the standard distribution.

@duckinator
Copy link
Owner

The current pylint setup is to enable everything, and disable what we don't want. I'm okay adding wrong-import-order to the list of disabled ones — I rarely find it useful. https://github.com/duckinator/bork/blob/master/.pylintrc#L21-L27

Also: the zipapp kludge is still needed, unfortunately, because the build library uses the pep517 library internally. We don't actually get rid of the pep517 dependency, we just get a better API to work with.

@duckinator
Copy link
Owner

@nbraud I'd say try reverting the commit removing the zipapp kludge and work from there.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 30, 2020

Also: the zipapp kludge is still needed, unfortunately, because the build library uses the pep517 library internally. We don't actually get rid of the pep517 dependency, we just get a better API to work with.

Oh sadness xD
I guess it only worked because I was testing in an environment where pep517 was installed.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 30, 2020

PS: Looks like the erroneous lint still triggers in the boopstrat tests.

@duckinator
Copy link
Owner

You can go ahead and add wrong-import-order to the list of disabled checkers here: https://github.com/duckinator/bork/blob/master/.pylintrc#L21-L27

I honestly thought I'd disabled it at some point. Must've been another project. 😛

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 30, 2020

OK, it looks like there are a few things going on:

  • FreeBSD and macOS have a legit test failure: error: invalid command 'bdist_wheel' when building ppb_mutant
    Is this because wheel isn't installed?
  • The zipapp bootstrap fails on linting, with an erroneous lint.

@nbraud nbraud force-pushed the build branch 3 times, most recently from c4878a8 to 5c7e01a Compare December 30, 2020 12:41
@nbraud
Copy link
Collaborator Author

nbraud commented Dec 30, 2020

@duckinator Could you look into the breakage on macOS? I don't have any Apple devices to test with :3

@duckinator
Copy link
Owner

@nbraud looks like we're running into pypa/build#108. Looks like the tl;dr is it's a resolved setuptools bug, and pinning setuptools>42 in pyproject.toml:

requires = ["setuptools > 42", "wheel"]

(We did similar with Emanate ages ago, for different reasons, in duckinator/emanate/pull/136)

This was referenced Jan 1, 2021
bors bot added a commit that referenced this pull request Jan 1, 2021
235: Fix CI r=nbraud a=nbraud

- [x] Stop running lints when running tests.
      Not only is this inefficient, it forces us to get linting working in all
      environments, which isn't currently possible (see pylint-dev/pylint#3882).
      As a result, remove the `_test-only` task.
- [x] Don't run tests when bootstrapping Bork in CI
      - It is useless, as `python3 /tmp/bork...pyz run test` immediately shells
        out to pytest, and tests the version of bork that is on the filesystem.
      - It fails because `git` isn't installed.

Changes by @duckinator and myself, extracted from #233 and #234

Co-authored-by: Ellen Marie Dash <[email protected]>
Co-authored-by: nicoo <[email protected]>
Using pep517 through its CLI entrypoint is not recommended.

Closes: #197
nbraud added 2 commits January 1, 2021 18:53
Otherwise, binary wheel packages cannot be built.
This is necessary, as `build` otherwise fails on macOS.
Older versions are buggy on macOS; see pypa/build#108
@nbraud
Copy link
Collaborator Author

nbraud commented Jan 1, 2021

Looks like version constraints in pyproject.toml's build-system.requires are silently ignored. It's ridiculous that, in 20 fucking 21, none of the Python tooling actually obeys version constraints, but it's somewhat out-of-scope for Bork to fix. >_>'

For now, I kicked in a setup step on macOS that updates pip and setuptools. I guess I should do that on all platforms, just to be sure?

Obsolete setuptools was the cause of a CI bug on macOS, see previous commit.
@duckinator
Copy link
Owner

duckinator commented Jan 2, 2021

It's ridiculous that, in 20 fucking 21, none of the Python tooling actually obeys version constraints

I know that pip is working on fixing this problem on their side. It was part of their big rewrite of the resolver, iirc.

I'll investigate and see if I can find what exact thing is responsible for ignoring the version here.

@duckinator
Copy link
Owner

duckinator commented Jan 2, 2021

For now, I kicked in a setup step on macOS that updates pip and setuptools. I guess I should do that on all platforms, just to be sure?

In theory we should be able to get away with just updating pip, which is probably good practice anyway. But yeah, since it's giving us problems, let's install new setuptools as well.

@duckinator
Copy link
Owner

Opened pypa/pip#9416 about being unable to pin requirement versions in pyproject.toml.

@nbraud
Copy link
Collaborator Author

nbraud commented Jan 2, 2021

Opened pypa/pip#9416 about being unable to pin requirement versions in pyproject.toml.

From the discussion there, it looks like pip honors the version constraints, but the projects being built in tests need that version constraint (for their build to succeed on macOS)

If you can reproduce the issue locally (I do not have a macOS box to test with), could you confirm that setuptools > 42 is sufficient to fix the issue? I can then go add the version constraint everywhere it's needed.

@duckinator
Copy link
Owner

@nbraud I do not have access to a mac, so can only reproduce it in CI. I'm not entirely sure how to approach this, tbh.

@nbraud
Copy link
Collaborator Author

nbraud commented Jan 3, 2021

@nbraud I do not have access to a mac, so can only reproduce it in CI. I'm not entirely sure how to approach this, tbh.

Oh, OK.

I'd suggest merging this anyhow, since the issue is shown to be outside of bork and the tests are working. Maybe we can enlist @AstraLuma's help to get this confirmed and fixed in the other projects?

@duckinator
Copy link
Owner

@nbraud sounds like a plan to me. merge when you think it's ready. 👍

@nbraud
Copy link
Collaborator Author

nbraud commented Jan 3, 2021

bors r=duckinator

@bors bors bot merged commit 6d2c896 into master Jan 3, 2021
@bors bors bot deleted the build branch January 3, 2021 21:48
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.

2 participants