-
Notifications
You must be signed in to change notification settings - Fork 120
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
build: validate the source directory passed to ProjectBuilder #260
Conversation
Fixes #259. Initial version, for some reason the |
I don't know why the tests are failing, they work locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think checking for either pyproject.toml
or setup.py
is fine, even though I don't like that people with a setup.cfg
will continue with the current behavior, making things slightly inconsistent, so I am not gonna block this.
src/build/__init__.py
Outdated
pyproject_toml = os.path.join(srcdir, "pyproject.toml") | ||
setup_py = os.path.join(srcdir, "setup.py") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not running pre-commit. Please use single quotes here 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Is the right way to run pre-commit to use the tox -e fix
command? As I said, that's failing for me :-( Is there a contributing guide that explains how to set up for development, run tests, etc? I wasn't able to find one.
I tried running it by hand, which seems to have worked. So I've pushed a fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should, I don't know exactly what went wrong in your case 😕. Perhaps @gaborbernat might have more insight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a contributor guide, but right now I'm ETIME.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I completely understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Is the right way to run pre-commit to use the
tox -e fix
command? As I said, that's failing for me
Can you provide more exact details for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to later, but basically it said I didn't have Visual C version 14 installed (I have VS 2019) when building the virtualenvs. But when I ran pre-commit outside of tox, it worked, and now tox -e fix
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. Was it trying to build some c-extension package (such as black?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I can't recall which one, I'll let you know once I manage to reproduce the issue.
You seem to have found an issue with our integration tests 😅. We test building a few popular projects in the CI as a sanity check and it looks like we are invoking I will open a separate PR, you can then rebase on top of it. |
Btw, as the integration tests take a long time and are only included as a sanity check, you need to pass Sorry for the trouble! |
This is why I'm not the biggest fan of not running integration tests locally by default 😆 People think if it passed locally they are done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic looks ok, but we need tests for these cases.
Yep, once the current tests pass I'll add tests for the new functionality. |
The tests should now be fixed now, @pfmoore can you rebase your branch on main? |
Thanks for the quick fix, rebase done. |
Thanks, when you come back to add the tests, could you also add an entry in the changelog? |
Done. By the way, the symlink from For portability, it might be better not to include symlinks in the repository... |
Uh, I thought symlinks were fixed on Windows, I guess that's not the case. |
I mean could be Paul doesn't have symlinks enabled 🤷♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this, it should be good! Thanks 😊
I do have symlinks enabled, and they do work. I suspect that the problem is that git might not handle them. I'm using a very vanilla git setup. |
@gaborbernat I've raised #262 with details of the pre-commit failure. @FFY00 I think I've done everything needed now. I've no idea why the coverage has gone down (or what I need to do to fix that). The coverage report seems to be flaggking lines that I haven't touched as uncovered 🙁 Let me know what (if anything) more I need to do. |
Thanks! The coverage is a bit fiddly, it is fine now. |
Thanks @FFY00 and @gaborbernat for the help getting this PR through 🙂 Much appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good.
@@ -136,6 +136,18 @@ def test_check_dependency(monkeypatch, requirement_string, expected): | |||
assert next(build.check_dependency(requirement_string), None) == expected | |||
|
|||
|
|||
def test_bad_project(test_no_project_path): | |||
# Passing a nonexistent project directory | |||
with pytest.raises(build.BuildException): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split each test into its own test please? 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code already went in. This is not a big deal though.
Sorry I missed this. It's also valid to have setup.cfg but no setup.py or pyproject.toml. |
It's not for legacy projects - how are you going to be able to invoke |
If there's no Also, see my comment here. In terms of PEP 517, there are only two sorts of source tree - ones with |
Not having a pyproject.toml is not something I do, but setup.cfg only projects are valid, and currently work in
This correctly produces a wheel and SDist, just like pip would. (Technically, I'm not sure how you would make an SDist without build, actually). Current master would now be broken. We suggest setup.cfg only projects on https://packaging.python.org/tutorials/packaging-projects/ (with a pyproject.toml too, of course, but technically it can be omitted and pip and current build work). |
Quick followup, pip checks this: python3 -m pip wheel .
ERROR: Directory '.' is not installable. Neither 'setup.py' nor 'pyproject.toml' found. So being just as picky in build is perfectly fine. Though it is a removal of something that used to work. |
As of pypa/pip#9945, we've now swapped with Pip. Pip 21.1.2 does support just setup.cfg now, and now we don't. 🤣 $ pip wheel .
ERROR: Directory '.' is not installable. Neither 'setup.py' nor 'pyproject.toml' found.
WARNING: You are using pip version 21.1.1; however, version 21.1.2 is available.
You should consider upgrading via the '/Users/henryschreiner/tmp/quickcheck/venv/bin/python3.9 -m pip install --upgrade pip' command.
$ pip install --upgrade pip
$ pip wheel .
Processing /Users/henryschreiner/tmp/quickcheck
DEPRECATION: A future pip version will change local packages to be built in-place without first copying to a temporary directory. We recommend you use --use-feature=in-tree-build to test your packages with this new behavior before it becomes the default.
pip 21.3 will remove support for this functionality. You can find discussion regarding this at https://github.com/pypa/pip/issues/7555.
Collecting numpy>=1.13.3
File was already downloaded /Users/henryschreiner/tmp/quickcheck/numpy-1.20.3-cp39-cp39-macosx_10_9_x86_64.whl
Building wheels for collected packages: quickcheck
Building wheel for quickcheck (setup.py) ... done
Created wheel for quickcheck: filename=quickcheck-0.0.0-py3-none-any.whl size=3372 sha256=57acfb84109eedde21404ffdea6ef0e87d6ef7663498e3220bb0c4a72b066d35
Stored in directory: /private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/pip-ephem-wheel-cache-62ksr1uu/wheels/9a/34/3d/e8d9298f424ae357bac078a56990fce39937c7ed9018594f47
Successfully built quickcheck |
No description provided.