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 pyproject toml #115

Merged
merged 8 commits into from
Oct 27, 2023
Merged

Switch to pyproject toml #115

merged 8 commits into from
Oct 27, 2023

Conversation

rnestler
Copy link
Member

No description provided.

@rnestler rnestler force-pushed the switch-to-pyproject-toml branch 4 times, most recently from c8fc172 to 5b0f284 Compare September 11, 2023 19:44
@rnestler rnestler requested a review from a team September 11, 2023 20:25
@rnestler rnestler force-pushed the switch-to-pyproject-toml branch from 5b0f284 to e63c2f8 Compare September 11, 2023 20:30
dependencies = [
"cadquery == 2.3.1",
]
version = "0.1.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

The version key is mandatory, even if it doesn't make much sense for us.

pyproject.toml Outdated
Comment on lines 5 to 10
{ name = "Hannes Badertscher", email = "[email protected]" },
{ name = "Murray", email = "[email protected]" },
{ name = "ouabache", email = "[email protected]" },
{ name = "Raphael Nestler", email = "[email protected]" },
{ name = "Tubbles", email = "[email protected]" },
{ name = "U. Bruhin", email = "[email protected]" },
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove most contributors and just leave @dbrgn and my as the main authors. U. Bruhin explicitly requested to be removed.

If anyone wants to add themself, they can just do it in a PR and we'll gladly merge it.

@rnestler rnestler removed the request for review from a team September 11, 2023 20:37
@rnestler rnestler force-pushed the switch-to-pyproject-toml branch 2 times, most recently from ced5630 to 6b40228 Compare September 11, 2023 20:46
@rnestler rnestler marked this pull request as ready for review September 11, 2023 20:50
@rnestler rnestler self-assigned this Sep 11, 2023
@rnestler rnestler requested a review from a team September 11, 2023 20:52
@ubruhin
Copy link
Member

ubruhin commented Sep 13, 2023

Would it be possible to also configure mypy so we can easily run it locally the same way as on CI?

https://github.com/LibrePCB/librepcb-parts-generator/blob/dc0e4bde936b6c9c84c8ce4092e8a1af4def152d/.github/workflows/main.yml#L23C11-L30

@rnestler
Copy link
Member Author

Would it be possible to also configure mypy so we can easily run it locally the same way as on CI?

Yes I think so.

@rnestler rnestler force-pushed the switch-to-pyproject-toml branch from 6b40228 to e6bb292 Compare September 30, 2023 10:37
@rnestler
Copy link
Member Author

Would it be possible to also configure mypy so we can easily run it locally the same way as on CI?

@ubruhin Done. You can now just call mypy . and all configuration is done in the pyproject.toml file. Also we now typecheck all files and just have less strict options.

@rnestler rnestler requested a review from ubruhin September 30, 2023 10:46
@ubruhin
Copy link
Member

ubruhin commented Sep 30, 2023

Nice! Just one problem: When generating some parts, the output directory out/ is created. While this directory exists, pip install .[test] fails with this error:

Processing librepcb-parts-generator
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error
  
  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [14 lines of output]
      error: Multiple top-level packages discovered in a flat-layout: ['out', 'entities'].
      
      To avoid accidental inclusion of unwanted files or directories,
      setuptools will not proceed with this build.
      
      If you are trying to create a single distribution with multiple packages
      on purpose, you should not rely on automatic discovery.
      Instead, consider the following options:
      
      1. set up custom discovery (`find` directive with `include` or `exclude`)
      2. use a `src-layout`
      3. explicitly set `py_modules` or `packages` with a list of names
      
      To find more information, look for "package discovery" on setuptools docs.
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.

@rnestler
Copy link
Member Author

rnestler commented Oct 1, 2023

Hmm. The proper solution would be to restructure completely to fit a python package style with on folder for the source code. The workaround would be to just exclude some folders manually.

I excluded the out directory for now.

@ubruhin
Copy link
Member

ubruhin commented Oct 1, 2023

OK the out directory does not cause any issues anymore. But now I realized that pip install .[test] causes a build/ directory to be created, with all our scripts duplicated into that directory and thus reported by Git as untracked files.

Looks like somehow a package will be created during this command although this repository does not represent a Python package. @rnestler Are you really sure the pyproject.toml is the right thing for us? Is there actually something wrong with requirements.txt? 🙈

@rnestler
Copy link
Member Author

rnestler commented Oct 9, 2023

OK the out directory does not cause any issues anymore. But now I realized that pip install .[test] causes a build/ directory to be created, with all our scripts duplicated into that directory and thus reported by Git as untracked files.

Hmm I tested it on my machine and it didn't create a build directory. But on the other hand: I think I used :pip install -e .[test] (-e being the important part). But we can also just add build to .gitignore.

Looks like somehow a package will be created during this command although this repository does not represent a Python package.

I'd argue that it should, since it would make stuff easier.

Are you really sure the pyproject.toml is the right thing for us? Is there actually something wrong with requirements.txt? 🙈

I guess the upsides are:

  • One standard place to configure dependencies and project tools
  • The order in which we specify the dependencies doesn't matter
  • Proper resolving of the whole dependency tree at once
  • Easy to distinguish between runtime and development dependencies
  • Encourages us to have a cleaner folder structure to conform to PyPA standards

Downsides are:

  • We either need to use some workarounds or use a proper Python package structure

@ubruhin
Copy link
Member

ubruhin commented Oct 16, 2023

Hmm I tested it on my machine and it didn't create a build directory. But on the other hand: I think I used :pip install -e .[test] (-e being the important part).

Ah true, -e helps 👍

Copy link
Member

@ubruhin ubruhin left a comment

Choose a reason for hiding this comment

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

At least it seems to work fine so I think I'm fine with it :)

rnestler and others added 5 commits October 27, 2023 11:18
Sadly flake8 doesn't support pyproject.toml. We could consider moving to
ruff.
We'll just mention the main authors for now. If anyone wants to be added
they can just make a PR to do so.
We now test all files, just some test modules with less strict options.
@rnestler rnestler force-pushed the switch-to-pyproject-toml branch from 8b93d05 to d0469bc Compare October 27, 2023 09:18
@rnestler rnestler enabled auto-merge October 27, 2023 09:18
@rnestler rnestler merged commit 9bfe94a into master Oct 27, 2023
8 checks passed
@rnestler rnestler deleted the switch-to-pyproject-toml branch October 27, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants