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

added 'build' subcmd to __main__.py #158

Merged
merged 11 commits into from
Oct 2, 2023
Merged

added 'build' subcmd to __main__.py #158

merged 11 commits into from
Oct 2, 2023

Conversation

shakfu
Copy link
Contributor

@shakfu shakfu commented Oct 1, 2023

This change adds a build subcmd to __main__.py and a zig_build_config function in buildzig.py for configuring builds from the command line (i.e. without using pyproject.toml).

This essentially this enables the following:

virtualenv venv && source venv/bin/activate
git clone https://github.com/shakfu/ziggy-pydust
cd ziggy-pydust
pip install -e .
cd ../demo # see attached demo
python -m pydust build --ext-name 'fib._lib' --ext-path 'fib/fib.zig'

Note that although the extension is successfully built and works via direct testing, at the end of the build an error/warning is output: "Not a valid python module, skipping... fib._lib No module named 'fib'"

demo.zip

@shakfu
Copy link
Contributor Author

shakfu commented Oct 1, 2023

Note that although the extension is successfully built and can be tested as such, at the end of build an error/warning is output: "Not a valid python module, skipping... fib._lib No module named 'fib'"

This issue is resolved if generate_stubs.py is copied into the containing folder:

python generate_stubs.py fib._lib .

which generates _lib.pyi in fib:

from __future__ import annotations

def nth_fibonacci_iterative(n, /): ...
def nth_fibonacci_recursive(n, /, *, f0=0, f1=1): ...
def nth_fibonacci_recursive_tail(n, /): ...

class Fibonacci:
    def __init__(first_n, /):
        pass
    def __iter__(self, /):
        """
        Implement iter(self).
        """
        ...

class FibonacciIterator:
    def __init__(i, ith, next, stop, /):
        pass
    def __next__(self, /):
        """
        Implement next(self).
        """
        ...

@shakfu
Copy link
Contributor Author

shakfu commented Oct 1, 2023

Added generate_stubs options to to build subcmd in __main__.py such that this works now:

python -m pydust build --generate-stubs --ext-name 'fib._lib' --ext-path 'fib/fib.zig'

@shakfu shakfu mentioned this pull request Oct 1, 2023
@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

Added a Builder class in __init__.py to serve as a frontend class for both programmatic usage and also command line usage.

@shakfu shakfu changed the title added build subcmd to __main__ for direct config builds added Builder frontend class Oct 2, 2023
@gatesn
Copy link
Contributor

gatesn commented Oct 2, 2023

Ah this is quite neat, thank you!

Do you think that this could be restructured around the current config object though? By that I mean, leave the arg parsing in main.py, and with the build subcommand, populate a ToolPydust object and pass that into zig_build.

So def zig_build(argv: list[str]): turns into def zig_build(argv: list[str], config: ToolPydust):

@shakfu shakfu changed the title added Builder frontend class added 'build' subcmd to __main__.py Oct 2, 2023
@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

Do you think that this could be restructured around the current config object though? By that I mean, leave the arg parsing in main.py, and with the build subcommand, populate a ToolPydust object and pass that into zig_build.

So def zig_build(argv: list[str]): turns into def zig_build(argv: list[str], config: ToolPydust):

Sure. While I personally prefer the class with command line api of the penultimate commit, I appreciate you want to keep the current functional api.

The latest commit provides the api as per your request, such that zig_build is now def zig_build(argv: list[str], conf: config.ToolPydust | None)

@shakfu shakfu mentioned this pull request Oct 2, 2023
Comment on lines 38 to 39
if not conf:
conf = config.load()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not conf:
conf = config.load()
conf = conf or config.load()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for the review.

@@ -12,15 +12,40 @@
limitations under the License.
"""

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Our copyright plugin screwed up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gatesn
Copy link
Contributor

gatesn commented Oct 2, 2023

Ah sorry, you got busted by Ruff

@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

Ah sorry, you got busted by Ruff

Ok that's fixed 😄

@gatesn gatesn enabled auto-merge (squash) October 2, 2023 15:17
@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

@gatesn I'm still getting a CI build failure. Could this be due to the stub_generation issue highlighted earlier?

@gatesn
Copy link
Contributor

gatesn commented Oct 2, 2023

Hmm, are you unable to see the output of CI? https://github.com/fulcrum-so/ziggy-pydust/actions/runs/6381967387/job/17319854114?pr=158

Looks like you're missing a conf argument in ziggy-pydust/test/conftest.py:23

auto-merge was automatically disabled October 2, 2023 15:30

Head branch was pushed to by a user without write access

@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

Hmm, are you unable to see the output of CI? https://github.com/fulcrum-so/ziggy-pydust/actions/runs/6381967387/job/17319854114?pr=158

Looks like you're missing a conf argument in ziggy-pydust/test/conftest.py:23

Thanks, I'll have a look at fixing this.

@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

@gatesn

The issue was this def zig_build(argv: list[str], conf: config.ToolPydust | None) instead of def zig_build(argv: list[str], conf: config.ToolPydust | None = None).

That's fixed in the latest commit

@gatesn gatesn merged commit d29dde6 into spiraldb:develop Oct 2, 2023
1 check passed
@shakfu
Copy link
Contributor Author

shakfu commented Oct 2, 2023

@gatesn Thanks for the merge!

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.

None yet

2 participants