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

feat: Support multiple extras in requirements constraint advertisement #379

Merged
merged 6 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
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
- Fail packaging if requirements are spelled differently in different places
- Fail packaging if requirements are named differently in different places or have different extras added


2023-08-11
**********

Expand Down
28 changes: 27 additions & 1 deletion python-template/{{cookiecutter.placeholder_repo_name}}/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,43 @@ def load_requirements(*requirements_paths):
with -c in the requirements files.
Returns a list of requirement strings.
"""
by_canonical_name = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment here with an example entry would be helpful. It took me a minute to figure out how it was being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, maybe something like this:

    # e.g. {"django": "Django", "openedx-events": "openedx_events"}


def check_name_consistent(package):
"""
Raise exception if package is spelled different ways.

This ensures that packages are spelled consistently so we can match
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really spelling.

Suggested change
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.)
"""
canonical = package.lower().replace('_', '-').split('[')[0]
seen_spelling = by_canonical_name.get(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,...")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update the comment to include how the regex treats [] ? it's a pretty hard regex to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you feel about using verbose mode? Not something I think I've ever used before, but I agree that this regex is... pretty dense.

    requirement_line_regex = re.compile(
        r"""
          (
            # The base name of the package
            [%s]+
            # Optionally, an extras section, as in `some-package[extra-one, extra-two]`
            (?:
              \[[%s,\s]+\]
            )?
          )

          # Optionally, a version constraint like `>=3.0`
          (
            [<>=][^#\s]+
          )?
        """
        % (re_package_name_base_chars, re_package_name_base_chars),
        re.VERBOSE
    )

Or even using a raw format-string, which is apparently a thing you can do? And maybe adding some spaces inside some of the more confusing character classes:

    requirement_line_regex = re.compile(
        fr"""
          (
            # The base name of the package
            [ {re_package_name_base_chars} ]+
            # Optionally, an extras section, as in `some-package[extra-one, extra-two]`
            (?:
              \[ [{re_package_name_base_chars},\s]+ \]
            )?
          )

          # Optionally, a version constraint like `>=3.0`
          (
            [<>=][^#\s]+
          )?
        """,
        re.VERBOSE
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I've pushed up a far simpler option. Verbose mode is cool but... probably overkill here.)

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.
Expand Down
4 changes: 2 additions & 2 deletions scripts/update_setup_py.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
27 changes: 26 additions & 1 deletion scripts/update_setup_py_load_requirements.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.get(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.
Expand Down