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

Move to src-layout. #2798

Merged
merged 24 commits into from
Nov 11, 2023
Merged

Move to src-layout. #2798

merged 24 commits into from
Nov 11, 2023

Conversation

apollo13
Copy link
Contributor

The intention of this is better isolation. trio is no longer accidentally on the (python) path and as such requires an explicit installation for usage, this helps uncover issues in packaging data etc early on (see
https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/).

This serves as the basis for switching to another build system as well; refs #2790.

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #2798 (3b582ca) into master (b7d2054) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2798   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files         115      115           
  Lines       17551    17551           
  Branches     3149     3149           
=======================================
  Hits        17408    17408           
  Misses         99       99           
  Partials       44       44           
Files Coverage Δ
src/trio/__init__.py 100.00% <ø> (ø)
src/trio/_abc.py 100.00% <ø> (ø)
src/trio/_channel.py 100.00% <ø> (ø)
src/trio/_core/__init__.py 100.00% <ø> (ø)
src/trio/_core/_asyncgens.py 100.00% <ø> (ø)
src/trio/_core/_entry_queue.py 100.00% <ø> (ø)
src/trio/_core/_exceptions.py 100.00% <ø> (ø)
src/trio/_core/_instrumentation.py 100.00% <ø> (ø)
src/trio/_core/_io_common.py 100.00% <ø> (ø)
src/trio/_core/_io_epoll.py 100.00% <ø> (ø)
... and 105 more

@apollo13 apollo13 force-pushed the src-layout branch 7 times, most recently from 3476c36 to 80ef0d6 Compare September 13, 2023 14:44
@altendky
Copy link
Member

In CI this is already addressed by changing into a subdirectory when testing. I personally prefer src/, but not doing that has been an intentional decision thus far. Though, I forget where I saw this discussed. :[ I'm just suggesting that the discussion is had since I don't see a reference from here to it..

@altendky
Copy link
Member

trio/ci.sh

Lines 106 to 110 in c16003f

# We run the tests from inside an empty directory, to make sure Python
# doesn't pick up any .py files from our working dir. Might have been
# pre-created by some of the code above.
mkdir empty || true
cd empty

@apollo13
Copy link
Contributor Author

@altendky I see, I will still finish this for now and put it up for discussion, because while CI might address it, it can still be pretty much an annoyance. Let's see what the rest says :)

The intention of this is better isolation. `trio` is no longer
accidentally on the (python) path and as such requires an explicit
installation for usage, this helps uncover issues in packaging data etc
early on (see
https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/).

This serves as the basis for switching to another build system as well; refs python-trio#2790.
@jakkdl
Copy link
Member

jakkdl commented Sep 13, 2023

I think #274 is the only place where it's been discussed.

I personally think the pro's outweigh the cons though, and give another +1 on the src/ train.

@CoolCat467
Copy link
Contributor

I definitely agree, a src/ based layout would be much nicer, +1

@jakkdl
Copy link
Member

jakkdl commented Oct 1, 2023

@njsmith do you want to weigh in on the matter?

@jakkdl
Copy link
Member

jakkdl commented Oct 10, 2023

@A5rocks any opinion on the matter?

@A5rocks
Copy link
Contributor

A5rocks commented Oct 10, 2023

I'm neutral on this change cause while it does align with what most people do, so would e.g. not shipping tests... so I don't think that's a very convincing argument.

And while it would take a bit of magic away from the tests it would also mean having an extra "src" folder in the path for every file in trio. (Then of course, I already have to type out trio/trio for the folder...)

So yeah arguments for and against but none of them feel strong, so neutral.

@jakkdl
Copy link
Member

jakkdl commented Oct 15, 2023

if this makes switching packaging system easier, I think we can just go ahead with it. The PRs been open a month and the only issue that has been brought up are minor workflow ones.

@CoolCat467
Copy link
Contributor

@apollo13 I would be glad to finish working on getting this merged this if you would like

@apollo13
Copy link
Contributor Author

@CoolCat467 That would be great, I am currently blocked by personal things. My goal with the PR was to be as minimal as possible. Ie moving _tests (if wanted at all) into another top level package would be something done after this PR (same for switching to whatever other tooling). I can give you access to my repo if you want to work on that branch directly or we close this PR and you open your own.

@apollo13
Copy link
Contributor Author

if this makes switching packaging system easier, I think we can just go ahead with it. The PRs been open a month and the only issue that has been brought up are minor workflow ones.

I guess it doesn't make switching much easier (if at all), tooling can usually handle both.

@altendky
Copy link
Member

Ie moving _tests (if wanted at all) into another top level package

I don't want that. :] I like the stuff I import to have been installed so it's not all fiddly about how you launch Python or what your cwd is. Also, why not be able to run tests on deployed packages? Also also, fewer directories to both including and excluding from all the myriad of tools and packaging definitions etc.

@CoolCat467
Copy link
Contributor

@CoolCat467 That would be great, I am currently blocked by personal things. My goal with the PR was to be as minimal as possible. Ie moving _tests (if wanted at all) into another top level package would be something done after this PR (same for switching to whatever other tooling). I can give you access to my repo if you want to work on that branch directly or we close this PR and you open your own.

If I could have access to the repo that would be great, I would prefer not creating another PR if it could be avoided.

@apollo13
Copy link
Contributor Author

apollo13 commented Oct 21, 2023 via email

@A5rocks
Copy link
Contributor

A5rocks commented Nov 3, 2023

OK release is out. Still ambivalent but if people think this would better and there isn't dissent I guess this should happen? Even if we don't end up using a pep 621 backend that requires the src/ dir, it still might be better to stick to the beaten path here.

(Obviously conflicts need to be fixed)

Copy link
Contributor

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly maybe in warning of "don't review your own PRs" since I've taken over continued work on this, but all the important changes were already made before that point and the tests are passing.

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to wait a day to see if anybody else pipes up about trio 0.23 being broken and needing a hotfix patch, but otherwise I don't see any problems with merging this.

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicts again :(

But mainly just that new exciting pyproject.toml changes mean you have to migrate some of the changes here.

.coveragerc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to move this to pyproject.toml

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you're still creating .coveragerc

setup.py Outdated Show resolved Hide resolved
@CoolCat467
Copy link
Contributor

CoolCat467 commented Nov 6, 2023

@A5rocks How do we fix this error?

  + coverage report -m --rcfile ../pyproject.toml
  No data to report.
  Error: Process completed with exit code 1.

All the tests pass, but this doesn't.

@A5rocks
Copy link
Contributor

A5rocks commented Nov 6, 2023

Make sure that coverage is outputting its data files in the cwd, maybe? Not sure tbh

@CoolCat467
Copy link
Contributor

@A5rocks I fixed the coverage report and resolved conflicts

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub says I have a pending review so let's see what it is...

Huh posting that before I reloaded was erroring. Strange. But whatever.

Just one concern then let's merge this!

pyproject.toml Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member

jakkdl commented Nov 11, 2023

let's not delay this any further and fix any other potential errors as they crop up. 🚀

@jakkdl jakkdl merged commit 315a0e8 into python-trio:master Nov 11, 2023
29 checks passed
@trio-bot
Copy link

trio-bot bot commented Nov 11, 2023

Hey @apollo13, it looks like that was the first time we merged one of your PRs! Thanks so much! 🎉 🎂

If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:

  • Github will automatically subscribe you to notifications on all our repositories. (But you can unsubscribe again if you don't want the spam.)

  • You'll be able to help us manage issues (add labels, close them, etc.)

  • You'll be able to review and merge other people's pull requests

  • You'll get a [member] badge next to your name when participating in the Trio repos, and you'll have the option of adding your name to our member's page and putting our icon on your Github profile (details)

If you want to read more, here's the relevant section in our contributing guide.

Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you.

If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out!

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

Successfully merging this pull request may close these issues.

None yet

5 participants