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

Add Python packaging infrastructure. #89

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

licquia
Copy link

@licquia licquia commented Jan 22, 2024

With this, it's now possible to build wheels, upload to PyPI if so desired, and install into a Python environment directly from GitHub.

Derived from work done by William Bartholomew [email protected].

With this, it's now possible to build wheels, upload to PyPI if
so desired, and install into a Python environment directly from
GitHub.

Derived from work done by William Bartholomew <[email protected]>.

Signed-off-by: Jeff Licquia <[email protected]>
@zvr
Copy link
Member

zvr commented Jan 22, 2024

Thanks for converting the previous PR to the new parser!

I’m not sure I understand the value, since I assume that spec-parser is only useful to a handful of people, all deeply involved in the SPDX project. I don’t object to the PR, provided that it doesn’t change the functionality of what is there.

Can you describe what would change in the setup and invocation of spec-parser ?

We currently have two modes of operation:
only checks (e.g. on the model repo)

git clone --depth=1 --single-branch --branch=main https://github.com/spdx/spec-parser /tmp/spec-parser
python3 /tmp/spec-parser/main.py -n some/where/.../model

output generation (e.g. on the spec repo)

git clone --depth=1 --single-branch --branch=main https://github.com/spdx/spec-parser /tmp/spec-parser
python3 -m pip install -r /tmp/spec-parser/requirements.txt
python3 /tmp/spec-parser/main.py some/where/.../model some/where/else/.../output_dir

How will these look like after this has been merged?

Please note that the first mode (checking functionality) does not depend on any other module, so it makes no sense to install all the prerequisites for RDF, Jinja templating, etc. in this case.

Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

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

I have questions on the implementation, but let's clear up whether it's worth it first.

[project]
name = "spec_parser"
version = "0.1.0"
dependencies = [
Copy link
Member

Choose a reason for hiding this comment

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

these dependencies are only for generating output; they are not needed when simply checking the input.

Copy link
Author

Choose a reason for hiding this comment

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

Is there value in leaving them out?

If you're wanting to avoid even hassling with installation when you just want to check, you can still run the main.py file; it's just done differently now. I can add docs to the README to mention that if we want.

spec-parser = "spec_parser.main:main"

[tool.setuptools.package-data]
"spec_parser.templates.mkdocs" = ["*.j2"]
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to have everything under templates included? Or do we have to add new lines here for each new directory therein? FYI, different generation formats will use different directories (e.g. I already have a onefile for when a single file has to be generated, plus some others).

Copy link
Author

Choose a reason for hiding this comment

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

I can change it to look for everything under templates if we want.

@@ -3,12 +3,16 @@
# SPDX-License-Identifier: Apache-2.0

from spec_parser import Model
from runparams import RunParams
from spec_parser.runparams import RunParams
Copy link
Member

Choose a reason for hiding this comment

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

why move it? RunParams are for processing command-line options. Not really anything to do with the spec-parsing itself.

Copy link
Author

Choose a reason for hiding this comment

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

This is to avoid namespace collisions when installing the parser in a Python environment. If we're concerned about clarity surrounding purpose, we could move main.py and runparams.py into something like spec_parser.cmd. But that might make it more inconvenient to run main.py directly. Was there a reason we separated the run parameters into their own file if they have no use outside of the command line?


if __name__ == "__main__":

def main():
Copy link
Member

Choose a reason for hiding this comment

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

why create a function? Is it something setuptools require?

Copy link
Author

Choose a reason for hiding this comment

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

Yes; this is how you get a spec-parser command-line utility. You have to identify a function for Python to call when run as a command-line tool.

@licquia
Copy link
Author

licquia commented Jan 23, 2024

Talking about the value:

This change makes it easier for people new to the project to work with the code, and allows us to take advantage of tools that work with standard Python projects.

For example, when we build the spdx-spec CI for the 3.0 branch, instead of writing several steps for setting up the parser, we can simply call python3 -m pip -r requirements.txt with a reference in requirements.txt like this:

spec-parser @ git+https://github.com/spdx/spec-parser@main

We can also tie the parser to a particular version of the spec, so if we refactor how the parser works for 3.1, we have the option of tying 3.0 spec builds to the older parser without interfering with the newer spec by changing the @main on the end to point to a git commit, tag, branch, etc.

You also get a real command-line utility, with its own name, by doing it this way. If you install in your environment the normal Python way, you can now run the parser with spec-parser model model-out instead of python3 /path/to/main.py model model-out. This is more concise and friendlier.

And if you need to do something different (like installing in a Python virtualenv), you'll generally know how to change the directions to do what you want without having to guess, since it's the same thing you'd do in other contexts.

The biggest argument for this patch, IMHO, is that it makes the code friendlier for people to work with when they're doing things with the spec. People who don't know or care about Python can just follow the directions, which are going to be no different than the directions they see in other Python-based projects. And people who do know Python will see this and know that a number of other things will just work.

@licquia
Copy link
Author

licquia commented Jan 23, 2024

Discussed on the tech call. For the moment, we're going to defer on this until some more important issues are worked out. There's no urgency to this PR, so it can wait.

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