From 554dbc71a6e052a71caf4293ba90ae7f6def703a Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Wed, 9 Aug 2023 21:19:20 +0000 Subject: [PATCH 1/6] feat: Support multiple extras in requirements constraint advertisement Update the base template's setup.py to support the full syntax of package extras. - Support commas and whitespace inside extras declarations, e.g. `some_package[extra_one, extra_two]` - Raise exception if we spell packages differently in requirement and constraint files, including presence/absence of extras. --- CHANGELOG.rst | 9 ++++++ .../setup.py | 28 ++++++++++++++++++- .../update_setup_py_load_requirements.yaml | 27 +++++++++++++++++- 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1b8f0591..ef4d8c85 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,15 @@ Change Log This file loosely adheres to the structure of https://keepachangelog.com/, but in reStructuredText instead of Markdown. +2023-08-15 +********** + +Changed +======= + +- In setup.py, support advertising constraints on packages with multiple extras +- Fail packaging if requirements are spelled differently in different places + 2023-08-11 ********** diff --git a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py index 6e162cf9..71df11b1 100755 --- a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py +++ b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py @@ -36,17 +36,43 @@ def load_requirements(*requirements_paths): with -c in the requirements files. Returns a list of requirement strings. """ + by_canonical_name = {} + + def check_name_consistent(package): + """ + Raise exception if package is spelled different ways. + + This ensures that packages are spelled consistently so we can match + constraints to packages. It also ensures that if we require a package + with extras we don't constrain it without mentioning the extras (since + that too would interfere with matching constraints.) + """ + canonical = package.lower().replace('_', '-').split('[')[0] + seen_spelling = by_canonical_name[canonical] + if seen_spelling is None: + by_canonical_name[canonical] = package + elif seen_spelling != package: + raise Exception( + f'Encountered both "{seen_spelling}" and "{package}" in requirements ' + 'and constraints files; please use just one or the other.' + ) + requirements = {} constraint_files = set() # groups "pkg<=x.y.z,..." into ("pkg", "<=x.y.z,...") - requirement_line_regex = re.compile(r"([a-zA-Z0-9-_.\[\]]+)([<>=][^#\s]+)?") + re_package_name_base_chars = r"a-zA-Z0-9\-_." # chars allowed in base package name + requirement_line_regex = re.compile( + r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" + % (re_package_name_base_chars, re_package_name_base_chars) + ) def add_version_constraint_or_raise(current_line, current_requirements, add_if_not_present): regex_match = requirement_line_regex.match(current_line) if regex_match: package = regex_match.group(1) version_constraints = regex_match.group(2) + check_name_consistent(package) existing_version_constraints = current_requirements.get(package, None) # It's fine to add constraints to an unconstrained package, # but raise an error if there are already constraints in place. diff --git a/scripts/update_setup_py_load_requirements.yaml b/scripts/update_setup_py_load_requirements.yaml index e18564f2..4214f0fd 100644 --- a/scripts/update_setup_py_load_requirements.yaml +++ b/scripts/update_setup_py_load_requirements.yaml @@ -17,18 +17,43 @@ rules: Returns a list of requirement strings. """ # UPDATED VIA SEMGREP - if you need to remove/modify this method remove this line and add a comment specifying why. + by_canonical_name = {} + + def check_name_consistent(package): + """ + Raise exception if package is spelled different ways. + + This ensures that packages are spelled consistently so we can match + constraints to packages. It also ensures that if we require a package + with extras we don't constrain it without mentioning the extras (since + that too would interfere with matching constraints.) + """ + canonical = package.lower().replace('_', '-').split('[')[0] + seen_spelling = by_canonical_name[canonical] + if seen_spelling is None: + by_canonical_name[canonical] = package + elif seen_spelling != package: + raise Exception( + f'Encountered both "{seen_spelling}" and "{package}" in requirements ' + 'and constraints files; please use just one or the other.' + ) requirements = {} constraint_files = set() # groups "pkg<=x.y.z,..." into ("pkg", "<=x.y.z,...") - requirement_line_regex = re.compile(r"([a-zA-Z0-9-_.\[\]]+)([<>=][^#\s]+)?") + re_package_name_base_chars = r"a-zA-Z0-9\-_." # chars allowed in base package name + requirement_line_regex = re.compile( + r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" + % (re_package_name_base_chars, re_package_name_base_chars) + ) def add_version_constraint_or_raise(current_line, current_requirements, add_if_not_present): regex_match = requirement_line_regex.match(current_line) if regex_match: package = regex_match.group(1) version_constraints = regex_match.group(2) + check_name_consistent(package) existing_version_constraints = current_requirements.get(package, None) # It's fine to add constraints to an unconstrained package, # but raise an error if there are already constraints in place. From 75c253809769b8f818589a90f1f5e0b06324d6e1 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Mon, 14 Aug 2023 19:56:39 +0000 Subject: [PATCH 2/6] fixup! Avoid missing key issue; clean up stray backslash --- .../{{cookiecutter.placeholder_repo_name}}/setup.py | 2 +- scripts/update_setup_py.sh | 4 ++-- scripts/update_setup_py_load_requirements.yaml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py index 71df11b1..22c7bb6d 100755 --- a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py +++ b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py @@ -48,7 +48,7 @@ def check_name_consistent(package): that too would interfere with matching constraints.) """ canonical = package.lower().replace('_', '-').split('[')[0] - seen_spelling = by_canonical_name[canonical] + seen_spelling = by_canonical_name.get(canonical) if seen_spelling is None: by_canonical_name[canonical] = package elif seen_spelling != package: diff --git a/scripts/update_setup_py.sh b/scripts/update_setup_py.sh index b4e460f5..7f8d2245 100755 --- a/scripts/update_setup_py.sh +++ b/scripts/update_setup_py.sh @@ -50,8 +50,8 @@ fi echo -e "[ARCHBOM-1772](https://openedx.atlassian.net/browse/ARCHBOM-1772) Update setup.py to use constraint files when generating requirements files for packaging and distribution. PR generated automatically with Jenkins job cleanup-python-code. " > .git/cleanup-python-code-description -echo -e "\nResult of running \`python setup.py bdist_wheel\` before applying fix (in .egg-info/requires.txt)\: \n" >> .git/cleanup-python-code-description +echo -e "\nResult of running \`python setup.py bdist_wheel\` before applying fix (in .egg-info/requires.txt): \n" >> .git/cleanup-python-code-description cat ./update-setup-tmp/old_requires.txt >> .git/cleanup-python-code-description -echo -e "\nResult of running \`python setup.py bdist_wheel\` after applying fix (in .egg-info/requires.txt)\: \n" >> .git/cleanup-python-code-description +echo -e "\nResult of running \`python setup.py bdist_wheel\` after applying fix (in .egg-info/requires.txt): \n" >> .git/cleanup-python-code-description cat "$(pwd)/$wheel_dir/requires.txt" >> .git/cleanup-python-code-description rm -rf update-setup-tmp diff --git a/scripts/update_setup_py_load_requirements.yaml b/scripts/update_setup_py_load_requirements.yaml index 4214f0fd..8c0981ae 100644 --- a/scripts/update_setup_py_load_requirements.yaml +++ b/scripts/update_setup_py_load_requirements.yaml @@ -29,7 +29,7 @@ rules: that too would interfere with matching constraints.) """ canonical = package.lower().replace('_', '-').split('[')[0] - seen_spelling = by_canonical_name[canonical] + seen_spelling = by_canonical_name.get(canonical) if seen_spelling is None: by_canonical_name[canonical] = package elif seen_spelling != package: From b5c4888b3c798c2c8327b76357109a16a6eddc83 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 15 Aug 2023 16:40:46 +0000 Subject: [PATCH 3/6] fixup! various clarifications --- CHANGELOG.rst | 2 +- .../{{cookiecutter.placeholder_repo_name}}/setup.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ef4d8c85..b8498a4e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,7 +12,7 @@ Changed ======= - In setup.py, support advertising constraints on packages with multiple extras -- Fail packaging if requirements are spelled differently in different places +- Fail packaging if requirements are named differently in different places or have different extras listed 2023-08-11 ********** diff --git a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py index 22c7bb6d..13b4b304 100755 --- a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py +++ b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py @@ -36,13 +36,14 @@ def load_requirements(*requirements_paths): with -c in the requirements files. Returns a list of requirement strings. """ + # e.g. {"django": "Django", "openedx-events": "openedx_events"} by_canonical_name = {} def check_name_consistent(package): """ - Raise exception if package is spelled different ways. + Raise exception if package is named different ways. - This ensures that packages are spelled consistently so we can match + This ensures that packages are named consistently so we can match constraints to packages. It also ensures that if we require a package with extras we don't constrain it without mentioning the extras (since that too would interfere with matching constraints.) @@ -63,7 +64,7 @@ def check_name_consistent(package): # groups "pkg<=x.y.z,..." into ("pkg", "<=x.y.z,...") re_package_name_base_chars = r"a-zA-Z0-9\-_." # chars allowed in base package name requirement_line_regex = re.compile( - r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" + r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" # name[maybe,extras] and maybe constraint % (re_package_name_base_chars, re_package_name_base_chars) ) From 27da0695bbbae3d0a5d969437620f79689c80a89 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 15 Aug 2023 18:59:43 +0000 Subject: [PATCH 4/6] fixup! Line too long; copy changes into semgrep version --- .../{{cookiecutter.placeholder_repo_name}}/setup.py | 3 ++- scripts/update_setup_py_load_requirements.yaml | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py index 13b4b304..0aaa796a 100755 --- a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py +++ b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py @@ -63,8 +63,9 @@ def check_name_consistent(package): # groups "pkg<=x.y.z,..." into ("pkg", "<=x.y.z,...") re_package_name_base_chars = r"a-zA-Z0-9\-_." # chars allowed in base package name + # Two groups: name[maybe,extras], and optionally a constraint requirement_line_regex = re.compile( - r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" # name[maybe,extras] and maybe constraint + r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" % (re_package_name_base_chars, re_package_name_base_chars) ) diff --git a/scripts/update_setup_py_load_requirements.yaml b/scripts/update_setup_py_load_requirements.yaml index 8c0981ae..48108971 100644 --- a/scripts/update_setup_py_load_requirements.yaml +++ b/scripts/update_setup_py_load_requirements.yaml @@ -17,13 +17,15 @@ rules: Returns a list of requirement strings. """ # UPDATED VIA SEMGREP - if you need to remove/modify this method remove this line and add a comment specifying why. + + # e.g. {"django": "Django", "openedx-events": "openedx_events"} by_canonical_name = {} def check_name_consistent(package): """ - Raise exception if package is spelled different ways. + Raise exception if package is named different ways. - This ensures that packages are spelled consistently so we can match + This ensures that packages are named consistently so we can match constraints to packages. It also ensures that if we require a package with extras we don't constrain it without mentioning the extras (since that too would interfere with matching constraints.) @@ -43,6 +45,7 @@ rules: # groups "pkg<=x.y.z,..." into ("pkg", "<=x.y.z,...") re_package_name_base_chars = r"a-zA-Z0-9\-_." # chars allowed in base package name + # Two groups: name[maybe,extras], and optionally a constraint requirement_line_regex = re.compile( r"([%s]+(?:\[[%s,\s]+\])?)([<>=][^#\s]+)?" % (re_package_name_base_chars, re_package_name_base_chars) From 41eaa6aef432c50cb33c7d10569e0eb70c637cad Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 15 Aug 2023 20:30:41 +0000 Subject: [PATCH 5/6] fixup! Swap out example to include something with extras --- python-template/{{cookiecutter.placeholder_repo_name}}/setup.py | 2 +- scripts/update_setup_py_load_requirements.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py index 0aaa796a..8a43be9f 100755 --- a/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py +++ b/python-template/{{cookiecutter.placeholder_repo_name}}/setup.py @@ -36,7 +36,7 @@ def load_requirements(*requirements_paths): with -c in the requirements files. Returns a list of requirement strings. """ - # e.g. {"django": "Django", "openedx-events": "openedx_events"} + # e.g. {"django": "Django", "confluent-kafka": "confluent_kafka[avro]"} by_canonical_name = {} def check_name_consistent(package): diff --git a/scripts/update_setup_py_load_requirements.yaml b/scripts/update_setup_py_load_requirements.yaml index 48108971..1607f4f4 100644 --- a/scripts/update_setup_py_load_requirements.yaml +++ b/scripts/update_setup_py_load_requirements.yaml @@ -18,7 +18,7 @@ rules: """ # UPDATED VIA SEMGREP - if you need to remove/modify this method remove this line and add a comment specifying why. - # e.g. {"django": "Django", "openedx-events": "openedx_events"} + # e.g. {"django": "Django", "confluent-kafka": "confluent_kafka[avro]"} by_canonical_name = {} def check_name_consistent(package): From 22be77ad57d1b281edfb3a8120012533dc62b96f Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Wed, 16 Aug 2023 20:23:28 +0000 Subject: [PATCH 6/6] fixup! Update changelog date --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b8498a4e..7293ae7f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,7 +5,7 @@ Change Log This file loosely adheres to the structure of https://keepachangelog.com/, but in reStructuredText instead of Markdown. -2023-08-15 +2023-08-16 ********** Changed