Source dist #7262

stealthycoin · 2022-09-09T16:08:11Z

No description provided.

kyleknap

Looks like a good start. I did a preliminary pass through the implementation. I did not get to dive too much into the tests nor trying out the implementation yet. I'll try to get through that in the next pass.

.github/workflows/source-dist-tests.yml

MANIFEST.in

backends/build_system/__main__.py

backends/build_system/awscli_venv.py

requirements/bootstrap.txt

bin/aws_bash_completer

backends/build_system/exe.py

kyleknap

I just did a follow up pass in trying it all out locally. It was looking good. Just left some comments after trying it out a bit.

kyleknap · 2022-09-13T19:17:55Z

configure.ac

+[with_download_deps=no]
+)
+if test "$with_download_deps" = no; then
+     ${PYTHON} ${srcdir}/backends/build_system validate-env --artifact $with_install_type || AC_MSG_ERROR("Python dependencies not met")


It may be worth including as part of the output the suggestion of either install these dependencies yourself or rerun configure with the --with-download-deps flag

kyleknap · 2022-09-13T19:19:27Z

configure.ac

+AC_ARG_WITH(install_type,
+            AS_HELP_STRING([--with-install-type=@<:@system-sandbox|portable-exe@:>@],
+                           [Specify type of AWS CLI installation. Options are:
+                            "portable-exe", "system-sandbox" (default is "system-sandbox")]),


It may be worth expanding on what each of these install types entail and why to use it. Or if we can add a link to some reference that would be handy too.

kyleknap · 2022-09-13T19:21:34Z

configure.ac

+                           [Download all dependencies and use those when
+                            building the AWS CLI. If not specified, the dependencies
+                            (including all python packages) must be installed
+                            on your system]),


It may be worth noting some of the caveats with this flag as detailed in the proposal: https://github.com/aws/aws-cli/blob/v2/proposals/source-install.md#backwards-compatibility. Specifically, there is no guarantee that this flag will download all dependencies in future versions and generally there should be no expectation that you upgrade the version of the CLI to install and that flag making build/install process automatically work.

backends/build_system/validate_env.py

Makefile.in

codecov-commenter · 2022-09-14T04:01:26Z

Codecov Report

Merging #7262 (c31ebe5) into source-distribution (12a87f8) will not change coverage.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##           source-distribution    #7262   +/-   ##
====================================================
  Coverage                93.70%   93.70%           
====================================================
  Files                      351      351           
  Lines                    36171    36171           
  Branches                  5202     5202           
====================================================
  Hits                     33895    33895           
  Misses                    1652     1652           
  Partials                   624      624

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kyleknap

It's looking good. Just had some more comments after the latest rebase and looking more into the tests.

requirements/bootstrap.txt

requirements/bootstrap-lock.txt

backends/build_system/utils.py

kyleknap · 2022-09-29T19:07:59Z

backends/build_system/utils.py

+    dependency_block_re = re.compile(
+        r"dependencies = \[([\s\S]+?)\]", re.MULTILINE
+    )
+    extract_dependencies_re = re.compile(r'"(.+)"')
+    with open(ROOT_DIR / "pyproject.toml", "r") as f:
+        data = f.read()
+    raw_dependencies = dependency_block_re.findall(data)[0]
+    dependencies = extract_dependencies_re.findall(raw_dependencies)


We should probably reuse this logic from the pep517.py module. In general, I would not be opposed to having a backends/utils.py module for stuff this needs to be shared between this and the PEP517 hooks.

backends/build_system/validate_env.py

tests/backends/build_system/integration/test_makefile.py

requirements/bootstrap-lock.txt

kyleknap

Looks good. I mainly focused on the tests this round. I liked the environment abstraction you made for the integration tests.

backends/build_system/constants.py

tests/backends/build_system/fakepackage/setup.py

tests/backends/build_system/functional/test_aws_cli_venv.py

tests/backends/build_system/functional/test_utils.py

tests/backends/build_system/integration/test_build_system.py

tests/backends/build_system/integration/test_makefile.py

tests/backends/build_system/unit/test_install.py

kyleknap

It's looking good. I just posted some more smaller comments to help get this polished up so we can merge it to the feature branch.

backends/build_system/install.py

backends/build_system/utils.py

kyleknap · 2022-10-11T04:06:10Z

requirements/bootstrap-lock.txt

+pip==22.2.2 \
+    --hash=sha256:3fd1929db052f056d7a998439176d3333fa1b3f6c1ad881de1885c0717608a4b \
+    --hash=sha256:b61a374b5bc40a6e982426aede40c9b5a08ff20e640f5b56977f4f91fed1e39a
+    # via -r requirements/bootstrap.txt


Is there a particular reason we have these top-level bootstrap-lock.txt and bootstrap-win-lock.txt lock files? I would have assumed we would removed them as the ones that would be used are the ones in the download-deps directory.

requirements/download-deps/bootstrap.txt

requirements/portable-exe-extras-win-lock.txt

requirements/runtime-lock.txt

kyleknap · 2022-10-11T04:12:12Z

scripts/ci/install

-run('python -m build')
-wheel_dist = glob.glob(os.path.join('dist', '*.whl'))[0]
-run('pip install %s' % wheel_dist)
+run("pip install setuptools==57.5.0")


Let's make sure to remove this direct install and defer to our requirements files for dependencies.

scripts/ci/install-build-system

kyleknap

Looks good. Just had a really small clean up comment on print statements, but otherwise let's go ahead and squash this into a single commit.

kyleknap · 2022-10-20T16:44:27Z

backends/build_system/utils.py

+        if ' #' in line:
+            line = line[:line.find(' #')]
+        if line.endswith('\\'):
+            print('before', line)


Looks like a left over print statement here and two lines after. We should just grep to make sure all these files to make sure there are no unintended print statements.

Changes: * Add python interface for building from source under backends/build_system * Add autotools interface for calling the python build backend * Add requirements/lock files under requirements/ directory needed for building form source. * Add script for regenerating the requirements lock files from scratch. * add .gitignore entries for autotools

kyleknap

Looks good! 🚢

stealthycoin requested a review from kyleknap September 9, 2022 16:08

kyleknap reviewed Sep 12, 2022

View reviewed changes

kyleknap reviewed Sep 13, 2022

View reviewed changes

stealthycoin force-pushed the source-dist branch 4 times, most recently from 0ff0f36 to 54e85a7 Compare September 14, 2022 03:46

stealthycoin force-pushed the source-dist branch from 54e85a7 to e93b652 Compare September 14, 2022 05:42

stealthycoin force-pushed the source-dist branch from e93b652 to 1e1f8cb Compare September 25, 2022 21:16

stealthycoin mentioned this pull request Sep 26, 2022

Update v2 to use pyproject.toml #7287

Merged

stealthycoin force-pushed the source-dist branch 2 times, most recently from 118333a to fe1170e Compare September 28, 2022 21:10

kyleknap reviewed Sep 29, 2022

View reviewed changes

stealthycoin force-pushed the source-dist branch from 3b7f035 to 80cf441 Compare October 6, 2022 16:16

kyleknap reviewed Oct 7, 2022

View reviewed changes

stealthycoin force-pushed the source-dist branch 2 times, most recently from d511d64 to 32967eb Compare October 10, 2022 22:06

kyleknap reviewed Oct 11, 2022

View reviewed changes

stealthycoin force-pushed the source-dist branch 4 times, most recently from ba0a43f to 901a2d0 Compare October 14, 2022 19:26

kyleknap reviewed Oct 20, 2022

View reviewed changes

stealthycoin force-pushed the source-dist branch from 4297fec to 693ee5b Compare October 20, 2022 17:32

stealthycoin force-pushed the source-dist branch from 693ee5b to c31ebe5 Compare October 20, 2022 18:43

kyleknap approved these changes Oct 20, 2022

View reviewed changes

stealthycoin merged commit 8b6a2fb into aws:source-distribution Oct 20, 2022

stealthycoin mentioned this pull request Jan 17, 2023

[v2][source] Tracking issue for installing AWS CLI v2 from source #6785

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source dist #7262

Source dist #7262

stealthycoin commented Sep 9, 2022

kyleknap left a comment

kyleknap left a comment

kyleknap Sep 13, 2022

kyleknap Sep 13, 2022

kyleknap Sep 13, 2022

codecov-commenter commented Sep 14, 2022 •

edited

Loading

kyleknap left a comment

kyleknap Sep 29, 2022

kyleknap left a comment

kyleknap left a comment

kyleknap Oct 11, 2022

kyleknap Oct 11, 2022

kyleknap left a comment

kyleknap Oct 20, 2022

kyleknap left a comment

Source dist #7262

Source dist #7262

Conversation

stealthycoin commented Sep 9, 2022

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap Sep 13, 2022

Choose a reason for hiding this comment

kyleknap Sep 13, 2022

Choose a reason for hiding this comment

kyleknap Sep 13, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2022 • edited Loading

Codecov Report

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap Sep 29, 2022

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap Oct 11, 2022

Choose a reason for hiding this comment

kyleknap Oct 11, 2022

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap Oct 20, 2022

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2022 •

edited

Loading