Skip to content

Conversation

@jb3
Copy link
Member

@jb3 jb3 commented Oct 18, 2025

This PR makes a start at decoupling the specific setup of snekbox PyDis uses and snekbox as a standalone application.

This is done by building a basic snekbox application that supports evaluation using only the system version of Python.

We then have a new separate Dockerfile.pydis that uses the pre-built versions from python-discord/python-builds.

An integration specific image using this Dockerfile is built for integration tests to mirror the current behaviour.

@jb3 jb3 force-pushed the jb3/pydis-version-separation branch 7 times, most recently from 558c6b3 to 77685ac Compare October 18, 2025 18:17
@coveralls
Copy link

coveralls commented Oct 18, 2025

Coverage Status

coverage: 88.265%. remained the same
when pulling d08f0bc on jb3/pydis-version-separation
into 28c95c8 on main.

@jb3 jb3 force-pushed the jb3/pydis-version-separation branch 2 times, most recently from 05a0526 to f118990 Compare October 18, 2025 18:25
@jb3 jb3 marked this pull request as ready for review October 18, 2025 18:48
@jb3 jb3 requested review from Den4200 and MarkKoz as code owners October 18, 2025 18:48
jb3 added 6 commits October 18, 2025 21:20
@jb3 jb3 force-pushed the jb3/pydis-version-separation branch from 73a4cd0 to d08f0bc Compare October 18, 2025 20:20
Comment on lines +15 to +19
services:
registry:
image: registry:2
ports:
- 5000:5000
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of hosting a registry in our job?

Copy link
Member Author

Choose a reason for hiding this comment

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

Having a registry allows us to use the newly built ghcr.io/python-discord/snekbox image for our snekbox-pydis build (you will see later that images are pushed to localhost:5000).

https://stackoverflow.com/a/63927832

There is probably some combination of cache-to/cache-from that may also work here but creating a local registry is a 3 second operation that then ensure we are using the right images and there will be no attempt made to pull images if the cache operations fail for whatever reason.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot that steps within the same job don't share the cache. But I thought that's what the existing cache-to/from already solve.

Comment on lines +16 to +19
"--nsjail-args",
nargs="*",
default=[],
dest="nsjail_args",
Copy link
Member

Choose a reason for hiding this comment

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

What does the help output look like now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Old:

root@snekbox-6c47cc4bf5-9gchm:/snekbox# python -m snekbox --help
usage: snekbox [-h] code [nsjail_args ...] [--- py_args ...]

positional arguments:
  code         the Python code to evaluate
  nsjail_args  override configured NsJail options (default: [])
  py_args      arguments to pass to the Python process (default: ['-c'])

options:
  -h, --help   show this help message and exit

New:

root@a7cdb21501e0 /snekbox
❯ python -m snekbox --help
usage: snekbox [-h] code [nsjail_args ...] [--- py_args ...]

positional arguments:
  code                  the Python code to evaluate

options:
  -h, --help            show this help message and exit
  --nsjail-args [NSJAIL_ARGS ...]
                        override configured NsJail options (default: []) (default: [])
  --py-args [PY_ARGS ...]
                        arguments to pass to the Python process (default: ['-c']) (default: ['-c'])

I can try find a solution that brings them closer together, they are functionally the same (hence no updates in the test cases) but the help is presented differently.

Copy link
Member

@MarkKoz MarkKoz Oct 26, 2025

Choose a reason for hiding this comment

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

Why is the default shown twice now?

Copy link
Member

Choose a reason for hiding this comment

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

They're now options rather than positional arguments. That's confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is the default shown twice now?

We hard-coded it previously but now switching it from store_const appears to display it again, we can remove our hardcoding.

They're now options rather than positional arguments. That's confusing.

Yeah, I'll be honest I'd just updated it to maintain functionality because argparse in 3.14 broke it, hadn't looked at how it changed the help command.

I'll have a play around to see if I can maintain behaviour on 3.14 and keep the help command useful.

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.

3 participants