-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: Replace usage of Pants with uv using workspaces #478
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 61 out of 75 changed files in this pull request and generated 1 comment.
Files not reviewed (14)
- BUILD: Language not supported
- scripts/get-pants.sh: Language not supported
- src/edx_sysadmin/BUILD: Language not supported
- src/edx_sysadmin/api/BUILD: Language not supported
- src/edx_sysadmin/conf/locale/BUILD: Language not supported
- src/edx_sysadmin/management/commands/BUILD: Language not supported
- src/edx_sysadmin/migrations/BUILD: Language not supported
- src/edx_sysadmin/settings/BUILD: Language not supported
- src/edx_sysadmin/static/edx_sysadmin/css/BUILD: Language not supported
- src/edx_sysadmin/static/edx_sysadmin/js/BUILD: Language not supported
- src/edx_sysadmin/templates/edx_sysadmin/BUILD: Language not supported
- src/edx_sysadmin/templatetags/BUILD: Language not supported
- src/edx_sysadmin/utils/BUILD: Language not supported
- src/edx_username_changer/BUILD: Language not supported
Comments suppressed due to low confidence (1)
pyproject.toml:64
- The project now requires Python >=3.11, so the ruff target version should be updated to reflect the current Python version (e.g., 'py311').
target-version = "py38"
5dfdf06
to
51486a9
Compare
# Extract members from root pyproject.toml (requires toml parser like yq or similar, or hardcode) | ||
# For simplicity, listing them explicitly based on pyproject.toml: | ||
PACKAGES=( | ||
"src/edx_sysadmin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tomllib comes bundled with Python now. Bet you could do this with a one liner and avoid the DRY violation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go :)
import tomllib; pypr=open("pyproject.toml","rb"); t=tomllib.load(pypr); members = t['tool']['uv']['workspace']['members']; print([ member.removeprefix('src/') for member in members ])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial thoughts. Now moving on to testing each plugin’s installation and functionality in edx-platform.
To build a package locally (e.g., for testing): | ||
```bash | ||
# Install build tool (if needed) | ||
# python -m pip install build uv | ||
# Navigate to the repository root | ||
cd /path/to/your/repo | ||
# Build the desired package (replace <plugin_dir> with the actual directory) | ||
python -m build src/<plugin_dir> | ||
# Or using uv (if installed) | ||
# uv build src/<plugin_dir> | ||
``` | ||
The built wheel and sdist will be placed in `src/<plugin_dir>/dist/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we move these instructions to open-edx-plugins/docs/README.rst and remove all the Pants-related steps in the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests aren’t running because the run_edx_integration_tests.sh
script is likely looking for a test directory at the top level of the plugin. Since we’ve moved the plugin files and tests into a subdirectory, the script no longer finds them. We should update the run_edx_integration_tests.sh script to reflect the new structure so the tests are properly discovered and executed.
The Pants build tool adds unnecessary complexity to this project. The core use case is multiple packages being able to be built and published which is easier to do with uv.
Co-authored-by: Copilot <[email protected]>
c4bd83a
to
d70a293
Compare
d70a293
to
0357ab9
Compare
What are the relevant tickets?
N/A
Description (What does it do?)
This replaces usage of Pants with UV for building and publishing the sub-packages. It also replaces Poetry for version and dependency management.
How can this be tested?
Run
uv build --all-packages
to ensure packages buildVerify that the packages install and the GH Actions complete successfully
Verify that the package metadata is consistent or better between the packages built by Pants and the ones build by UV
Verify that the expected files are still present in the package and the namespaces are properly maintained.
Additional Context
The Pants build tool adds unnecessary complexity to this project. The core use case is multiple packages being able to be built and published which is easier to do with uv.