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

Adds a script for updating vendored code in sunpy/extern #6127

Merged
merged 44 commits into from
May 25, 2022

Conversation

akash5100
Copy link
Contributor

@akash5100 akash5100 commented May 6, 2022

PR Description

This PR is an attempt to close #6094

This PR will add a script in the sunpy/tools that will be used to update vendor code automatically, by the person doing a release. Executing this script will update all python files in sunpy/sunpy/extern.

@akash5100 akash5100 marked this pull request as draft May 6, 2022 08:35
Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

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

Hi @akash5100, thank you for working on this PR! Looking through your script, it seems that you are attempting to update the dependencies of the packages in sunpy/sunpy/extern rather than the packages (sunpy/sunpy/extern/*.py) themselves.

As an example, updating sunpy/sunpy/extern/appdirs.py should download the latest released version of the upstream ActiveState/appdirs appdirs.py. (Either using the GitHub API or pip download appdirs for example.) Then the script should replace the old appdirs.py file with the upstream file (and possibly run SunPy's precommit hooks to fix any PEP8 issues etc. not enforced upstream).

@nabobalis nabobalis added the Infrastructure Issues or PRs that affect the CI or packaging. label May 6, 2022
@akash5100
Copy link
Contributor Author

akash5100 commented May 6, 2022

Oh sorry! I misunderstood, thank you for the example @ConorMacBride.

@akash5100
Copy link
Contributor Author

akash5100 commented May 8, 2022

I think this script will fail in 2 cases (maybe more):

  1. If you've ever had to use the GitHub REST APIs, you may be familiar with GitHub's API Rate Limits:
    This means that if you call a GitHub API more than 5,000 times within a 60-minute window, even if you are authenticated, the GitHub APIs will return 403 FORBIDDEN.

  2. If the packages are released without a version tag.

edit: I don't know much about release of a package, but If pip download <package> always gives us the latest version, it would be the solution.

@nabobalis
Copy link
Contributor

I think this script will fail in 2 cases (maybe more):

  1. If you've ever had to use the GitHub REST APIs, you may be familiar with GitHub's API Rate Limits:
    This means that if you call a GitHub API more than 5,000 times within a 60-minute window, even if you are authenticated, the GitHub APIs will return 403 FORBIDDEN.

I don't think we have to worry about that.

  1. If the packages are released without a version tag.

This might be an issue.

edit: I don't know much about release of a package, but If pip download <package> always gives us the latest version, it would be the solution.

What does pip download do?

tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
@akash5100
Copy link
Contributor Author

What does pip download do?

This command will download the .whl file of the package in the given directory.

For example, pip download distro will download distro-1.7.0-py3-none-any.whl (python wheel file) in the same directory, we then rename the extension from .whl to .zip, after that same procedure. (extracting, moving)

tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
Co-authored-by: Conor MacBride <[email protected]>
@akash5100
Copy link
Contributor Author

akash5100 commented May 23, 2022

I think we also should copy over the latest license file for each package, which should be stored in the dist.

If we have a list of files that we need to extract, we can do this in the same function, but I need to figure out how we going to rename the files.

If we have a list of files that we need to extract

Something like this:

PACKAGES = {
    "appdirs": ["ActiveState", "appdirs", ["appdirs.py", "LICENSE.txt" ],
}

Otherwise, without any change in the current function, we can make a new dictionary of LICENSE and call it once again? (But this will repeat downloading, extracting, deleting)

@nabobalis
Copy link
Contributor

We just rename it to <app_name>_licence?

@akash5100
Copy link
Contributor Author

We just rename it to <app_name>_licence?

Adding this does the work without changing the function, does this looks good?

LICENSES = {
    "appdirs_license.txt": ["ActiveState", "appdirs", "LICENSE.txt"],
    "distro_license.rst": ["python-distro", "distro", "LICENSE"],
    "inflect_license.txt": ["jaraco", "inflect", "LICENSE"],
    "parse_license.txt": ["r1chardj0n3s", "parse", "LICENSE"],
}

for package, (user, repo, src) in LICENSES.items():
    download_github_file(user, repo, src, SUNPY_EXTERN_DIR / package)

@ConorMacBride
Copy link
Member

We just rename it to <app_name>_licence?

Might need to update the dictionary keys to be appdirs.py etc, and then add the licences as new "packages" appdirs_licence.txt etc to the dictionary.

@ConorMacBride
Copy link
Member

Just seeing your new comment now. I think the separate LICENSES dictionary looks much neater! And you can leave PACKAGES as-is.

@nabobalis
Copy link
Contributor

But wont it download the archive twice?

@akash5100
Copy link
Contributor Author

akash5100 commented May 23, 2022

Actually, we can add .py in the keys instead of formatting the string while calling the function, still works the same.

PACKAGES = {
    "appdirs.py": ["ActiveState", "appdirs", "appdirs.py"],
    "distro.py": ["python-distro", "distro", "src/distro/distro.py"],
    "inflect.py": ["jaraco", "inflect", "inflect/__init__.py"],
    "parse.py": ["r1chardj0n3s", "parse", "parse.py"],
}

and then

    for package, (user, repo, src) in PACKAGES.items():
        download_github_file(user, repo, src, SUNPY_EXTERN_DIR / package)

If we have the LICENSES dict, we can just replace PACKAGES.items() to LICENSES.item()

@ConorMacBride
Copy link
Member

Oh! We don't have to download the whole repo! We should be able to just download the single file: https://raw.githubusercontent.com/{user}/{repo}/{version}/{src} (e.g. https://raw.githubusercontent.com/ActiveState/appdirs/1.4.4/appdirs.py)

Then we can keep download_github_file as a function which only downloads one file.

@akash5100
Copy link
Contributor Author

akash5100 commented May 23, 2022

Nice, no more zips

edit: Sorry, I missed this, all these days downloading and extracting zips. But, I wonder, if this endpoint is not in the documentation how do you find it?

@ConorMacBride
Copy link
Member

In GitHub if you navigate to a file with a particular tag selected e.g. https://github.com/sunpy/sunpy/blob/v4.0.0/setup.py you can see this URL by clicking the "Raw" button. I think we should be able to rely on this? I have seen this type of URL used directly before. What do you think @nabobalis?

@ConorMacBride
Copy link
Member

Actually, that URL is documented so this should be fine. https://docs.github.com/en/repositories/creating-and-managing-repositories/about-repositories#text-limits

@nabobalis
Copy link
Contributor

In GitHub if you navigate to a file with a particular tag selected e.g. https://github.com/sunpy/sunpy/blob/v4.0.0/setup.py you can see this URL by clicking the "Raw" button. I think we should be able to rely on this? I have seen this type of URL used directly before. What do you think @nabobalis?

Sounds good

@nabobalis nabobalis dismissed ConorMacBride’s stale review May 24, 2022 00:02

Lots of new changes!

@akash5100 akash5100 marked this pull request as draft May 24, 2022 05:38
@akash5100
Copy link
Contributor Author

Then we can keep download_github_file as a function which only downloads one file.

We get bytes as a response, Should I write the byte in the required file?
for example: write the response directly in sunpy/extern/appdirs.py

@nabobalis
Copy link
Contributor

Then we can keep download_github_file as a function which only downloads one file.

We get bytes as a response, Should I write the byte in the required file? for example: write the response directly in sunpy/extern/appdirs.py

Sure.

@akash5100 akash5100 marked this pull request as ready for review May 24, 2022 19:09
@akash5100 akash5100 requested a review from nabobalis May 24, 2022 19:09
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
tools/update_extern.py Outdated Show resolved Hide resolved
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Seems sane to me. Thanks!

@nabobalis
Copy link
Contributor

nabobalis commented May 25, 2022

SQUASH MERGE

@nabobalis nabobalis merged commit a4dbe0b into sunpy:main May 25, 2022
@nabobalis
Copy link
Contributor

Thanks for the PR @akash5100

@akash5100 akash5100 deleted the extern_script branch June 5, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues or PRs that affect the CI or packaging. No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate updating vendored code in sunpy/extern
4 participants