[DEPR]: Paver #34467
Comments
All CI used to go through scripts/generic-ci-tests.sh, which is a wrapper around various `paver` test/linting/check invocations. These days, most edx-platform CI checks just invoke their tools (pylint, pycodestyle, pytest, etc.) directly. In anticipation of the proposed Paver deprecation [1], let's remove the parts of this script that aren't used any more, including several `paver` command invocations. This should have no impact on CI. Furthermore, we are able to remove the SHARD environment variable, which was formely used to split unit and quality checks up into smaller pieces. Unit tests and pylint checks now have their own separate sharding logic, so there is only one "quality" shard remaining (SHARD=4, ie generic quality checks), thus we don't need a SHARD variable at all. [1] openedx#34467
All CI used to go through scripts/generic-ci-tests.sh, which is a wrapper around various `paver` test/linting/check invocations. These days, most edx-platform CI checks just invoke their tools (pylint, pycodestyle, pytest, etc.) directly. In anticipation of the proposed Paver deprecation [1], let's remove the parts of this script that aren't used any more, including several `paver` command invocations. This should have no impact on CI. Furthermore, we are able to remove the SHARD environment variable, which was formely used to split unit and quality checks up into smaller pieces. Unit tests and pylint checks now have their own separate sharding logic, so there is only one "quality" shard remaining (SHARD=4, ie generic quality checks), thus we don't need a SHARD variable at all. [1] #34467
) All CI used to go through scripts/generic-ci-tests.sh, which is a wrapper around various `paver` test/linting/check invocations. These days, most edx-platform CI checks just invoke their tools (pylint, pycodestyle, pytest, etc.) directly. In anticipation of the proposed Paver deprecation [1], let's remove the parts of this script that aren't used any more, including several `paver` command invocations. This should have no impact on CI. Furthermore, we are able to remove the SHARD environment variable, which was formely used to split unit and quality checks up into smaller pieces. Unit tests and pylint checks now have their own separate sharding logic, so there is only one "quality" shard remaining (SHARD=4, ie generic quality checks), thus we don't need a SHARD variable at all. [1] openedx#34467
Together, these changes make it so that all features of the Paver-based asset compilation system are supported with drop-in Paver-free replacements. The remaining Paver asset functions are trivial wrappers, which can be comfortably deleted before Sumac * Turn `./manage.py ... compile_sass` into a simple wrapper around `npm run compile-sass` * Turn `paver webpack` into a simple wrapper around `npm run webpack` * Turn `pavelib.assets:collect_assets` into a simple wrapper around `./manage.py ... collectstatic` * Add/improve deprecation warnings for all Paver asset commands. * Load defaults for asset-related Django settings from environment variables. This allows the build to work without Python. For the settings which will be removed in Sumac, I've added deprecation warnings. * Change EDX_PLATFORM_THEME_DIRS env var to COMPREHENSIVE_THEME_DIRS. This simplifies the migration instructions, because all the new env vars now match their corresponding Django settings. This amends an ADR, but it should not be a breaking change because the env var was recently added (since Quince) and nobody should be using it yet. * Future-proof the static assets ADR with links. The linked pages will be kept up-to-date even if the ADR isn't. Part of: openedx#34467
Together, these changes make it so that all features of the Paver-based asset compilation system are supported with drop-in Paver-free replacements. The remaining Paver asset functions are trivial wrappers, which can be comfortably deleted before Sumac. * Turn `./manage.py ... compile_sass` into a simple wrapper around `npm run compile-sass` * Turn `paver webpack` into a simple wrapper around `npm run webpack` * Turn `pavelib.assets:collect_assets` into a simple wrapper around `./manage.py ... collectstatic` * Add/improve deprecation warnings for all Paver asset commands. * Load defaults for asset-related Django settings from environment variables. This allows the build to work without Python. For the settings which will be removed in Sumac, I've added deprecation warnings. * Change EDX_PLATFORM_THEME_DIRS env var to COMPREHENSIVE_THEME_DIRS. This simplifies the migration instructions, because all the new env vars now match their corresponding Django settings. This amends an ADR, but it should not be a breaking change because the env var was recently added (since Quince) and nobody should be using it yet. * Future-proof the static assets ADR with links. The linked pages will be kept up-to-date even if the ADR isn't. Part of: #34467
|
I have updated the Django settings to clarify that we will also be removing the STATIC_ROOT_BASE Django setting. |
This is the first step of Paver removal. These are the parts that we are confident are unused. Part of: openedx#34467
TODO describe merge timing concerns Part of: openedx#34467
This is the first step of Paver removal. These are the parts that we are confident are unused. Part of: openedx#34467
|
What @feanil says is right. I've tried to amend the description to be a bit clearer. In retrospect, it may have been clearer to just have two DEPR tickets, but doing that now would probably just add confusion. @timmc-edx let me know if it's still unclear. |
|
I see now, thanks! |
|
Just putting it here as a note for something that I/someone else could do: would be nice to create a make command for running asset processing, because otherwise I will forget the correct way of doing it. |
FWIW, it's just Also there is a full guide in the docs: https://github.com/kdmccormick/edx-platform/blob/master/docs/references/static-assets.rst |
|
Oh, awesome! I have been around too long and of course I didn't think to read the doc. 😅 |
|
Friendly reminder that the breaking changes listed in the ticket description under "Operators: ACTION REQUIRED" are slated to land as early as 09 Nov. |
These packages were installed transitively through paver.in, but they are used as direct dependencies in edx-platform application code: * psutil * pymemcache * wrapt Since we are demoting paver.in to be a dev-only dependency (with plans to remove paver.in entirely), we need to make those three packages explicit dependencies in kernel.in Part of: openedx#34467
TODO details Part of: openedx#34467
These packages were installed transitively through paver.in, but they are used as direct dependencies in edx-platform application code: * psutil * pymemcache * wrapt Since we are demoting paver.in to be a dev-only dependency (with plans to remove paver.in entirely), we need to make those three packages explicit dependencies in kernel.in Part of: openedx#34467
TODO details Part of: openedx#34467
|
Hi @kdmccormick Could you guide me a bit about the replacement of these commands. Thanks in advance. |
|
@UsamaSadiq
None of that should matter for edx/configuration, correct? Are you asking because the commands are referenced in docs/concepts/testing/testing.rst? If so, that is a good point--we will need to update that documentation. |
@kdmccormick yes, similar to the dead script We're almost done with the clearance on our end so could we make the paver removal change on Monday? This'll give us some bandwidth to wrap up internal config as well. |
|
Sure thing @UsamaSadiq |
|
Hi @kdmccormick Thanks for extending the cut off time for us. We're done with removing all occurrences of paver from 2U configs so you can now proceed with doing the final removals on your end. Thanks. |
Part of: openedx#34467
Part of: openedx#34467
Part of: openedx#34467
These packages were installed transitively through paver.in, but they are used as direct dependencies in edx-platform application code: * psutil * pymemcache * wrapt Since we are demoting paver.in to be a dev-only dependency (with plans to remove paver.in entirely), we need to make those three packages explicit dependencies in kernel.in Part of: openedx#34467
From the upstream Paver DEPR [1], this accounts for a change in the requirements files: > Starting in Sumac, these dependencies will be removed from > requirements/edx/base.txt. Instead, operators will need to install: > * requirements/edx/assets.txt to build static assets In the future, we could optimize the openedx image build by installing assets.txt in a separate, rarely-invalidated cache layer. libsass in particular takes 60+ seconds to install, so this is promising. However, for now, in order to prepare for [1], we make just the simplest possible change, which is to install assets.txt along with base.txt. We also take this opportunity to bump the nodeenv version from 1.8.0 to 1.9.1, matching edx-platform's. This is another area where we could make use of assets.txt in a future Dockerfile refactoring. [1] openedx/edx-platform#34467
BREAKING CHANGE: Removes all remaining Paver commands including `pavelib/prereqs.py:*` and `pavelib/assets.py:*`. BREAKING CHANGE: Removes `./manage.py [lms|cms] compile_sass`, which was just a wrapper around Paver commands. BREAKING CHANGE: Removes paver.txt. Operators should install testing.txt instead. Part of: openedx#34467
These packages were installed transitively through paver.in, but they are used as direct dependencies in edx-platform application code: * psutil * pymemcache * wrapt Since we are demoting paver.in to be a dev-only dependency (with plans to remove paver.in entirely), we need to make those three packages explicit dependencies in kernel.in Part of: openedx#34467
BREAKING CHANGE: Removes all remaining Paver commands including `pavelib/prereqs.py:*` and `pavelib/assets.py:*`. BREAKING CHANGE: Removes `./manage.py [lms|cms] compile_sass`, which was just a wrapper around Paver commands. BREAKING CHANGE: Removes paver.txt. Operators should install testing.txt instead. Part of: openedx#34467
These packages were installed transitively through paver.in, but they are used as direct dependencies in edx-platform application code: * psutil * pymemcache * wrapt Since we are demoting paver.in to be a dev-only dependency (with plans to remove paver.in entirely), we need to make those three packages explicit dependencies in kernel.in Part of: openedx#34467
ℹ️ This DEPR ticket considers the Paver commands in two groups:
Timeline
Communicated
Operator-facing commands: 2023-03-08 (thread)
Internal CI commands: 2024-05-01 (thread)
Acceptance
Operator-facing commands: 2023-05-03
Internal CI commands: 2024-05-21
Replacement available
Operator-facing commands: 2024-05-07, Redwood
Internal CI commands: ~2024-11-09, early Teak (Target)
Earliest removal
Operator-facing commands: 2024-11-09, early Teak
Internal CI commands: as soon as replacement is available
Rationale
edx-platform historically handled its build scripts with paver:
Using paver has a few problems:
paverpackage, this means that we installlibsass-pythonin production, which takes 60+ seconds (!) to compile and install.For the Paver Asset commands in particular, more depth is provided in the 'Reimplement edx-platform static asset processing' ADR.
Replacement
pylint, etc.) when possible, Makefile targets otherwisepipMigration
To ease migration for Paver users, in Redwood, each Paver Asset command will simply proxy to its replacement command, and will raise a deprecation warning explaining the new command that it is running.
By Nov 9th 2024, operators will need to have switched to the new commands, taking the steps indicated below by ACTION REQUIRED.
Requirements (Operators: ACTION REQUIRED)
In Redwood and earlier, Paver and its dependencies were included in requirements/edx/base.txt
Starting in Sumac, these dependencies will be removed from requirements/edx/base.txt. Instead, operators will need to install:
Operator-facing commands with known users (Operators: ACTION REQUIRED)
These replacements are production-ready as of May 7th, for Redwood.
paver update_assetsnpm run build && ./manage.py lms collectstatic --noinput && ./manage.py cms collectstaticpaver process_xmodule_assetspaver compile_sassnpm run compile-sass./manage.py [lms/cms] compile_sassnpm run compile-sasspaver webpacknpm run webpackpaver watch_assetsnpm run watchpaver install_prereqspip install -r requirements/edx/base.txt -r requirements/edx/assets.txt && npm clean-installpaver install_node_prereqsnpm clean-installpaver uninstall_python_packagespaver install_coverage_prereqspip install -r requirements/edx/coverage.txtpaver install_python_prereqspip install -r requirements/edx/base.txtOperator-facing commands without known users
These commands have been replaced for a long time. We don't know of any users of them, except the old Vagrant Devstack, which was deprecated 8 years ago.
paver build_docsmake docspaver i18n_validate_gettextwhich xgettextpaver i18n_extractmake extract_translationspaver i18n_dummyi18n_tool dummy && i18n_tool generatepaver i18n_generatei18n_tool generatepaver i18n_generate_stricti18n_tool generate --strictpaver i18n_cleanmake clean_translationspaver check_settingspaver lms./manage.py lms runserverpaver studio./manage.py cms runserverpaver run_all_serverstutor local ...paver devstacktutor dev ...Internal CI commands
These commands are only used by the upstream openedx project for CI. They will replaced before Sumac. Operators do not need to take action.
paver find_fixmepaver run_eslinteslintpaver run_stylelintmake stylelint_jspaver run_xsslintmake xsslintpaver run_pii_checkmake pii_checkpaver check_keywordsmake check_keywordspaver run_qualitypaver run_pylintpylintpaver run_pep8pycodestylepaver diff_coveragemake diff_coverage_jspaver test_jsmake test_jspaver test_js_runmake test_jspaver test_js_devmake test_js MODE=browserDjango settings for Asset commands (Operators: ACTION REQUIRED)
In order to reimplement the Paver Asset commands without Python/Django, we are changing how several settings are configured.
The following Django settings are becoming read-only mirrors. If you override them now, remove your overrides:
STATIC_ROOT(a string, loaded fromSTATIC_ROOT_LMSenv var)COMPREHENSIVE_THEME_DIRS(loaded from the env var, parsed into a list of strings)STATIC_ROOT(a string, loaded fromSTATIC_ROOT_CMSenv var)COMPREHENSIVE_THEME_DIRS(loaded from the env var, parsed into a list of strings)The following Django settings are being removed. If you override them now, remove your overrides:
STATIC_ROOT_BASEWEBPACK_CONFIG_PATHJS_ENV_EXTRA_CONFIGSTATIC_ROOT_BASEWEBPACK_CONFIG_PATHJS_ENV_EXTRA_CONFIGThe following new environment variables are available. Set these in your environment, in place of the overrides you removed above:
STATIC_ROOT_LMS(path)STATIC_ROOT_CMS(path)COMPREHENSIVE_THEME_DIRS(colon-separated paths)WEBPACK_CONFIG_PATH(path)JS_ENV_EXTRA_CONFIG(serialized as json)Note: If you previously set the
STATIC_ROOT_BASEDjango setting to/blah, then you should now set the LMS and CMS environment variables as so:STATIC_ROOT_LMS=/blahSTATIC_ROOT_CMS=/blah/studioDeprecation
In time for Redwood, deprecation warnings will be added to all edx-platform Paver Asset commands.
Removal
In edx-platform:
Elsewhere in the openedx GitHub org:
In Tutor:
Tasks/PRs
npm run build(experimental) #32823npm run watch(experimental) #32866The text was updated successfully, but these errors were encountered: