fix: Pip not resolving local packages by mildaniel · Pull Request #250 · aws/aws-lambda-builders

mildaniel · 2021-06-24T17:37:03Z

Which issue(s) does this change fix?
aws/aws-sam-cli#2705

Why is this change necessary?
Fix local dependencies not being resolved by pip.

How does it address the issue?
Looks like pip updated the output that we try to string match, meaning we weren't catching local packages for newer versions of pip.

This change adds multiple layers of regex matching to include both the old and new output format.

What side effects does this change have?

Checklist:

Add input/output type hints to new functions/methods
Write design document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

wchengru · 2021-06-24T20:38:08Z

aws_lambda_builders/workflows/python_pip/packager.py

    """Wrapper around pip calls used by chalice."""

-    _LINK_IS_DIR_PATTERN = "Processing (.+?)\n  Link is a directory, ignoring download_dir"
+    _LINK_IS_DIR_PATTERNS = ["Processing (.+?)\n  Link is a directory, ignoring download_dir", "Processing (.+?)\n"]


You can add some docs on why the pattern is changed.

Match the second one should always match the first. I believe you have already deduplicated this case, but you can simplify it.

If we only match "Processing sth.", will we introduce non-dir links?

I can definitely go ahead and add some docs.

It is true for this case, but in the future if the output is to change again, there's no guarantee that the new string will be a subset of the old one. Doing it this way should be more extensible and backwards compatible. It also prevents from having to re-write existing pip test stubs that explicitly write out Processing (.+?)\n Link is a directory, ignoring download_dir. However, if you think removing the Link is a directory, ignoring download_dir pattern is more appropriate, I can certainly go ahead and do that.

This was my main concern. Searching through the pip code, though, I think this should be okay as it doesn't appear to log this type of pattern anywhere else. This could always change, but that's the nature of this type of implementation.

For 2, You have done a good change on making it a list. You can just keep the second one since the first one is not necessary.

Won’t that break customers using an older version of pip?

I don't think it would because the matched group will be the same in both cases, and anything matched by Processing (.+?)\n Link is a directory, ignoring download_dir will also be matched by Processing (.+?)\n. I just tested this and it seems to be fine. If there is backward compatibility concern, I can also include the old pattern.

jpassaro · 2021-06-25T17:10:04Z

if this fix is preferred over #249, can I ask that you at least include the integration test case from that change?

mildaniel · 2021-06-25T23:41:49Z

if this fix is preferred over #249, can I ask that you at least include the integration test case from that change?

Hi @jpassaro. From a quick look, the integration test appears to be quite similar to the one in this pr. Is there something specific you are testing for in #249?

jpassaro · 2021-06-26T11:38:51Z

yes, that setup tests specifically the scenario where requirements.txt has just one entry, <sourcedir>, which has a python project with a src/ structure:

<sourcedir>/setup.py
<sourcedir>/setup.cfg
<sourcedir>/src/mycode/__init__.py
<sourcedir>/src/mycode/something.py

this was how I discovered the bug and I'd like to be sure that if a future change conflicts with this setup, the tests will catch it.

mildaniel · 2021-06-28T17:36:44Z

I've updated the integration test to match file structure you defined. If there is a similar breaking change in the future, it will catch it.

jpassaro · 2021-06-28T18:19:03Z

thanks, that meets my ask entirely. if it wasn't clear, I was suggesting to keep your test case (it seems useful to me) and add mine as an additional test; whether to do that or replace it as you've done is entirely up to y'all. Either way I really appreciate your attention.

jfuss

Looks good to me. Just a question on one of the tests.

jfuss · 2021-06-30T20:27:36Z

tests/functional/workflows/python_pip/test_packager.py

        pip, runner = pip_runner
        appdir, builder = self._make_appdir_and_dependency_builder(reqs, tmpdir, runner)
        requirements_file = os.path.join(appdir, "requirements.txt")
-        pip.set_return_tuple(0, (b"Processing ../foo\n" b"  Link is a directory," b" ignoring download_dir"), b"")


Wonder if we should keep thins so we know we don't break back compat? Or make this output from pip parameterized to test both cases?

Reverted those tests to keep using the old output string. Updated the unit tests to test a mixture of reading both new and old.

jpassaro · 2021-07-09T16:23:23Z

@wchengru @jfuss @mildaniel thanks for your work and attention on this. Any idea when this change will make it to a release of aws-sam-cli?

* feat: Allow Python runtime to build without requiring a manifest (#243) * Allow Python to continue build without requirements.txt * Allow missing requirements.txt file for Python builds * Ruby optional Gemfile and test * Style changes * Add unit tests and additional comments * Fix assertLogs() not compatible with Python2.7 * Fix assertion string * Remove unused exception * Integ. test make sure no artifacts dir has no additional files after build * Kick off build * Check for manifest and skip package actions if not found * Revert Ruby changes until further discussion is had. Make Python workflow more readable. * Remove unused import * Whitespace fix * feat: Allow Ruby runtime to build without requiring a manifest (#245) * Allow Python to continue build without requirements.txt * Allow missing requirements.txt file for Python builds * Ruby optional Gemfile and test * Style changes * Add unit tests and additional comments * Fix assertLogs() not compatible with Python2.7 * Fix assertion string * Remove unused exception * Integ. test make sure no artifacts dir has no additional files after build * Kick off build * Check for manifest and skip package actions if not found * Revert Ruby changes until further discussion is had. Make Python workflow more readable. * Remove unused import * Whitespace fix * Readability changes for Ruby workflow * Remove magic number. Add link to Bundler error codes * Moved var declaration * docs: Guidance on integrating with Lambda Builders (#242) * fix: README - showcase Makefile support (#247) * Update VS2017 to VS2019 (#244) * fix: Pip not resolving local packages (#250) * Fix local packages not being built * Add int. test to catch future local dependency issues * Specify test requirements path from cwd * Removed redundant/superset pattern * Document the pip regex pattern change * Updated integ to match use case * Tests to check backward comp. * chore: bump version to 1.5.0 (#254) Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: Giorgio Azzinnaro <giorgio.azzinnaro@gmail.com> Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com>

mildaniel added 2 commits June 23, 2021 17:20

Fix local packages not being built

fdba79c

Add int. test to catch future local dependency issues

e6f5415

mildaniel added the pr/internal label Jun 24, 2021

Specify test requirements path from cwd

19aec7b

wchengru reviewed Jun 24, 2021

View reviewed changes

Removed redundant/superset pattern

a9f026f

mgrandis changed the title ~~Fix pip not resolving local packages~~ fix: Pip not resolving local packages Jun 24, 2021

Document the pip regex pattern change

ccf9482

Updated integ to match use case

1a301e8

jfuss suggested changes Jun 30, 2021

View reviewed changes

jfuss mentioned this pull request Jun 30, 2021

Look for updated pip logging to detect directory links #249

Closed

Tests to check backward comp.

89870f9

wchengru approved these changes Jul 7, 2021

View reviewed changes

jfuss approved these changes Jul 8, 2021

View reviewed changes

mildaniel merged commit ec47987 into aws:develop Jul 8, 2021

jpassaro mentioned this pull request Jul 9, 2021

Python pip download action no longer builds local link into a wheel #246

Closed

moelasmar mentioned this pull request Jul 19, 2021

Build logic problem when package is specified as relative-path in requirements.txt for python 3.8 aws/aws-sam-cli#3063

Closed

This was referenced Jul 28, 2021

Merge develop into master (#255) #256

Closed

Merge master into develop (#255) #257

Closed

jfuss mentioned this pull request Aug 5, 2021

Allow python_pip builder to install relative packages #153

Closed

mildaniel deleted the Issue2705-local-packages branch September 22, 2021 19:16

mndeveci mentioned this pull request May 16, 2022

Referencing local libraries from parent directory fails quietly aws/aws-sam-cli#2705

Closed

Conversation

mildaniel commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wchengru Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

mildaniel Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

wchengru Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

jfuss Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

mildaniel Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

jpassaro commented Jun 25, 2021

Uh oh!

mildaniel commented Jun 25, 2021

Uh oh!

jpassaro commented Jun 26, 2021

Uh oh!

mildaniel commented Jun 28, 2021

Uh oh!

jpassaro commented Jun 28, 2021

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

jfuss Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

mildaniel Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

jpassaro commented Jul 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mildaniel commented Jun 24, 2021 •

edited

Loading