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

Fix: Remove unneeded directory "schema" #5753

Merged
merged 18 commits into from
Aug 30, 2023
Merged

Conversation

forzagreen
Copy link
Contributor

Which issue(s) does this change fix?

It fixes a bug introduced in v1.95.0:
When installing aws-sam-cli, it creates 2 folders in site-packages: samcli and schema.

Why is this change necessary?

The folder schema should not be published. It pollutes the environment, and enters in conflict with third-party library schema

How does it address the issue?

Remove the directory schema before publishing the package.

Mandatory Checklist

  • make pr passes

@forzagreen forzagreen requested a review from a team as a code owner August 11, 2023 08:53
@github-actions github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Aug 11, 2023
@forzagreen
Copy link
Contributor Author

I was not sure whether to remove the directory here:

echo "Removing CI Scripts and other files/direcories not needed"
rm -vf ./output/aws-sam-cli-src/appveyor*.yml
rm -rf ./output/aws-sam-cli-src/tests
rm -rf ./output/aws-sam-cli-src/designs
rm -rf ./output/aws-sam-cli-src/docs
rm -rf ./output/aws-sam-cli-src/media

or to exclude it here:
packages=find_packages(exclude=["tests.*", "tests", "installer.*", "installer"]),

@lucashuy
Copy link
Contributor

Thanks for raising this PR! We can remove this folder from our packages, however they shouldn't affect the PKG installers since those are isolated from the rest of the system, and thus from any other Python packages. We can exclude this folder anyways since the code that is in there is not used by the rest of the tool.

We can remove the folder in the setup.py file like you've noted so that it doesn't get included in the package that is uploaded to pypi.

@lucashuy lucashuy removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Aug 11, 2023
@forzagreen
Copy link
Contributor Author

@lucashuy thank you for your response. Feel free to review/update this PR, or to close it and open a fix in a separate PR.

@lucashuy
Copy link
Contributor

We should be able to reuse this existing PR. We can add that schema folder to the setup.py's exclude, and we can also rm that folder for the build-mac.sh script too. That way the behaviour is the same across our repo's installation scripts.

@forzagreen
Copy link
Contributor Author

@lucashuy , done.
build-mac.sh was missing rm of other folders, so I added the missing deletions to have the same behaviour as build-linux.sh

@forzagreen
Copy link
Contributor Author

Reverted adding schema to the setup.py's exclude, because it breaks make schema

Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

I believe the changes in setup.py are needed. build-linux.sh and build-mac.sh are used to create our installers. If you install AWS SAM CLI using our installers, I don't think you will hit the issue you saw. My guess is that you used pip install. But then if setup.py is not updated, using pip install will still hit the same issue.

I think the easiest way is to change the "schema" directory name to another name that's more unlikely to conflict with existing packages on PyPI.

@forzagreen
Copy link
Contributor Author

@hawflau , thank you for reviewing.
Indeed we got the issue when using pip install.

Do we really want to deploy the schema folder ? Isn't it used only for development purposes ?
If schema must be deployed ? why not just putting it inside samcli ? otherwise, I would suggest renaming it awscli-schema

@lucashuy
Copy link
Contributor

After some offline discussion, we can still exclude the folder and it's contents like was previously done in setup.py. To fix the issue of the file not being found when being installed in dev mode (like being done in the PR jobs), we can just change the import statements for the schema generation file itself.

This involves changing the follow line:

from schema.exceptions import SchemaGenerationException
to just an import exceptions and updating the two references to SchemaGenerationException in the same file, to exceptions.SchemaGenerationException. This should solve the issue of a missing parent module when excluding this folder from the setup file.

@forzagreen
Copy link
Contributor Author

import exceptions brings other issues in typing and unit tests. I simply modified PYTHONPATH in make schema @lucashuy @hawflau

@forzagreen
Copy link
Contributor Author

Fixed an issue with Windows build in eeb968b . I run make_schema.py with -m instead of setting PYTHONPATH.

CI tested in https://github.com/forzagreen/aws-sam-cli/actions/runs/5936100836

@lucashuy
Copy link
Contributor

Fixed an issue with Windows build in eeb968b . I run make_schema.py with -m instead of setting PYTHONPATH.

CI tested in https://github.com/forzagreen/aws-sam-cli/actions/runs/5936100836

Thanks for checking this out, this looks to be a cleaner fix!

@forzagreen
Copy link
Contributor Author

@lucashuy @hawflau Thank you for the review. Feel free to merge.

Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@hawflau hawflau enabled auto-merge August 24, 2023 16:08
@hawflau hawflau added this pull request to the merge queue Aug 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 24, 2023
@lucashuy lucashuy added this pull request to the merge queue Aug 30, 2023
Merged via the queue into aws:develop with commit 65268b9 Aug 30, 2023
76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants