Skip to content

fix: pyinstaller binaries - #4486

Merged
sriram-mv merged 2 commits into
aws:developfrom
sriram-mv:fix_pyinstaller_cfn_lint
Dec 15, 2022
Merged

fix: pyinstaller binaries#4486
sriram-mv merged 2 commits into
aws:developfrom
sriram-mv:fix_pyinstaller_cfn_lint

Conversation

@sriram-mv

@sriram-mv sriram-mv commented Dec 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

  • With the inclusion of cfn-lint as a dependency. There are data files, python files that need to be included for cfn-lint.

  • One of the dependencies brought in by cfn-lint is pbr, this in turn has a dependency on jschema-to-python. The way around it is to explictly set PBR_VERSION as per https://docs.openstack.org/pbr/latest/user/packagers.html. However this solution is brittle and will leak dependency code into AWS SAM CLI codebase, the cleaner solution is to package jschema-to-python metadata only for pyinstaller instead.

Which issue(s) does this change fix?

N/A

Why is this change necessary?

  • To get a working pyinstaller binary.

How does it address the issue?

What side effects does this change have?

  • None

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • 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.

@sriram-mv
fix: pyinstaller binaries
30bbe5b 
- With the inclusion of `cfn-lint` as a dependency. There are data
  files, python files that need to be included for `cfn-lint`.

- One of the dependencies brought in by `cfn-lint` is `pbr`, this in
  turn has a dependency on `jschema-to-python`. The way around is to
  explictly set `PBR_VERSION` as per
  https://docs.openstack.org/pbr/latest/user/packagers.html.
  However this solution is brittle and will leak dependency code into
  AWS SAM CLI codebase, the cleaner solution is to package it only for
  `pyinstaller`.
@sriram-mv
sriram-mv requested a review from a team as a code owner December 15, 2022 01:41
@sriram-mv
sriram-mv requested review from jfuss and mndeveci December 15, 2022 01:41
@qingchm

qingchm commented Dec 15, 2022

Copy link
Copy Markdown
Contributor

Is there a PR to add jschema_to_python into the hidden_imports list? Or do we only need the data files from jschema_to_python but not the library itself?

@qingchm

qingchm commented Dec 15, 2022

Copy link
Copy Markdown
Contributor

Also the same question goes for cfn-lint, do we only need cfn-lint.rules in https://github.com/aws/aws-sam-cli/blob/develop/installer/pyinstaller/hidden_imports.py or should we have the entire cfn-lint as a hidden import?

@sriram-mv

sriram-mv commented Dec 15, 2022

Copy link
Copy Markdown
Contributor Author

Yeah we only need the metadata files for jschema-to-python. the Library is a transitive dependency.

Good question on the second one, we do need the python files as well. Haven't tried playing around, I can do that tomorrow.

@sriram-mv
deps: cfnlint to be fully importable.
25113b5 
- `cfnlint` fully importable package and removed from `hidden_imports`.
- `jschema-to-python` only import data files and package metadata.
@sriram-mv
sriram-mv force-pushed the fix_pyinstaller_cfn_lint branch from cabf4b8 to 25113b5 Compare December 15, 2022 18:01
@sriram-mv
sriram-mv merged commit b8a939d into aws:develop Dec 15, 2022
mndeveci added a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 16, 2022
@mndeveci
Revert "fix: pyinstaller binaries (aws#4486)"
ffd74e7 
This reverts commit b8a939d.
mndeveci added a commit that referenced this pull request Dec 17, 2022
@mndeveci
revert: remove cfn-lint integration (#4494)
42e512d 
* Revert "fix: `hooks` data imports for pyinstaller (#4491)"

This reverts commit bea3bc0.

* Revert "fix: Update expected message read validate lint integration test (#4488)"

This reverts commit abd7c03.

* Revert "Update lint helpand output message (#4489)"

This reverts commit 3304955.

* Revert "fix: `pyinstaller` binaries (#4486)"

This reverts commit b8a939d.

* Revert "fix:  Fix validate command integration tests console output missmatch and update pyyaml version requirement (#4479)"

This reverts commit ce7143c.

* Revert "Adding cfn-lint as optional parameter for SAM validate command (#4444)"

This reverts commit 2fd533f.
mndeveci pushed a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@sriram-mv @mndeveci
fix: pyinstaller binaries (aws#4486)
63ec6f5 
* fix: `pyinstaller` binaries

- With the inclusion of `cfn-lint` as a dependency. There are data
  files, python files that need to be included for `cfn-lint`.

- One of the dependencies brought in by `cfn-lint` is `pbr`, this in
  turn has a dependency on `jschema-to-python`. The way around is to
  explictly set `PBR_VERSION` as per
  https://docs.openstack.org/pbr/latest/user/packagers.html.
  However this solution is brittle and will leak dependency code into
  AWS SAM CLI codebase, the cleaner solution is to package it only for
  `pyinstaller`.

* deps: cfnlint to be fully importable.

- `cfnlint` fully importable package and removed from `hidden_imports`.
- `jschema-to-python` only import data files and package metadata.
mndeveci added a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@mndeveci
revert: remove cfn-lint integration (aws#4494)
f5c9f44 
* Revert "fix: `hooks` data imports for pyinstaller (aws#4491)"

This reverts commit bea3bc0.

* Revert "fix: Update expected message read validate lint integration test (aws#4488)"

This reverts commit abd7c03.

* Revert "Update lint helpand output message (aws#4489)"

This reverts commit 3304955.

* Revert "fix: `pyinstaller` binaries (aws#4486)"

This reverts commit b8a939d.

* Revert "fix:  Fix validate command integration tests console output missmatch and update pyyaml version requirement (aws#4479)"

This reverts commit ce7143c.

* Revert "Adding cfn-lint as optional parameter for SAM validate command (aws#4444)"

This reverts commit 2fd533f.
mndeveci pushed a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@sriram-mv @mndeveci
fix: pyinstaller binaries (aws#4486)
ae94499 
* fix: `pyinstaller` binaries

- With the inclusion of `cfn-lint` as a dependency. There are data
  files, python files that need to be included for `cfn-lint`.

- One of the dependencies brought in by `cfn-lint` is `pbr`, this in
  turn has a dependency on `jschema-to-python`. The way around is to
  explictly set `PBR_VERSION` as per
  https://docs.openstack.org/pbr/latest/user/packagers.html.
  However this solution is brittle and will leak dependency code into
  AWS SAM CLI codebase, the cleaner solution is to package it only for
  `pyinstaller`.

* deps: cfnlint to be fully importable.

- `cfnlint` fully importable package and removed from `hidden_imports`.
- `jschema-to-python` only import data files and package metadata.
mndeveci added a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@mndeveci
revert: remove cfn-lint integration (aws#4494)
c3fc28b 
* Revert "fix: `hooks` data imports for pyinstaller (aws#4491)"

This reverts commit bea3bc0.

* Revert "fix: Update expected message read validate lint integration test (aws#4488)"

This reverts commit abd7c03.

* Revert "Update lint helpand output message (aws#4489)"

This reverts commit 3304955.

* Revert "fix: `pyinstaller` binaries (aws#4486)"

This reverts commit b8a939d.

* Revert "fix:  Fix validate command integration tests console output missmatch and update pyyaml version requirement (aws#4479)"

This reverts commit ce7143c.

* Revert "Adding cfn-lint as optional parameter for SAM validate command (aws#4444)"

This reverts commit 2fd533f.
mndeveci pushed a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@sriram-mv @mndeveci
fix: pyinstaller binaries (aws#4486)
384b4e2 
* fix: `pyinstaller` binaries

- With the inclusion of `cfn-lint` as a dependency. There are data
  files, python files that need to be included for `cfn-lint`.

- One of the dependencies brought in by `cfn-lint` is `pbr`, this in
  turn has a dependency on `jschema-to-python`. The way around is to
  explictly set `PBR_VERSION` as per
  https://docs.openstack.org/pbr/latest/user/packagers.html.
  However this solution is brittle and will leak dependency code into
  AWS SAM CLI codebase, the cleaner solution is to package it only for
  `pyinstaller`.

* deps: cfnlint to be fully importable.

- `cfnlint` fully importable package and removed from `hidden_imports`.
- `jschema-to-python` only import data files and package metadata.
mndeveci added a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@mndeveci
revert: remove cfn-lint integration (aws#4494)
c24af57 
* Revert "fix: `hooks` data imports for pyinstaller (aws#4491)"

This reverts commit bea3bc0.

* Revert "fix: Update expected message read validate lint integration test (aws#4488)"

This reverts commit abd7c03.

* Revert "Update lint helpand output message (aws#4489)"

This reverts commit 3304955.

* Revert "fix: `pyinstaller` binaries (aws#4486)"

This reverts commit b8a939d.

* Revert "fix:  Fix validate command integration tests console output missmatch and update pyyaml version requirement (aws#4479)"

This reverts commit ce7143c.

* Revert "Adding cfn-lint as optional parameter for SAM validate command (aws#4444)"

This reverts commit 2fd533f.
mndeveci pushed a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@sriram-mv @mndeveci
fix: pyinstaller binaries (aws#4486)
49637bd 
* fix: `pyinstaller` binaries

- With the inclusion of `cfn-lint` as a dependency. There are data
  files, python files that need to be included for `cfn-lint`.

- One of the dependencies brought in by `cfn-lint` is `pbr`, this in
  turn has a dependency on `jschema-to-python`. The way around is to
  explictly set `PBR_VERSION` as per
  https://docs.openstack.org/pbr/latest/user/packagers.html.
  However this solution is brittle and will leak dependency code into
  AWS SAM CLI codebase, the cleaner solution is to package it only for
  `pyinstaller`.

* deps: cfnlint to be fully importable.

- `cfnlint` fully importable package and removed from `hidden_imports`.
- `jschema-to-python` only import data files and package metadata.
mndeveci added a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 19, 2022
@mndeveci
revert: remove cfn-lint integration (aws#4494)
c7b8c56 
* Revert "fix: `hooks` data imports for pyinstaller (aws#4491)"

This reverts commit bea3bc0.

* Revert "fix: Update expected message read validate lint integration test (aws#4488)"

This reverts commit abd7c03.

* Revert "Update lint helpand output message (aws#4489)"

This reverts commit 3304955.

* Revert "fix: `pyinstaller` binaries (aws#4486)"

This reverts commit b8a939d.

* Revert "fix:  Fix validate command integration tests console output missmatch and update pyyaml version requirement (aws#4479)"

This reverts commit ce7143c.

* Revert "Adding cfn-lint as optional parameter for SAM validate command (aws#4444)"

This reverts commit 2fd533f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jfuss jfuss Awaiting requested review from jfuss jfuss was automatically assigned from aws/serverless-application-experience-sbt
2 more reviewers
@mndeveci mndeveci mndeveci approved these changes
@qingchm qingchm qingchm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr/internal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sriram-mv @qingchm @mndeveci