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

Expose all .whl,.ABOUT, .NOTICE,.LICENCE files and Create an archive #2117

Merged

Conversation

Abhishek-Dev09-zz
Copy link

@Abhishek-Dev09-zz Abhishek-Dev09-zz commented Jul 16, 2020

It will upload all .whl,.ABOUT, .NOTICE,.LICENCE files of thirdparty repo to new repo as an asset of specific OS and python and also generate archive for each Oses and all python

Release them as asset on Github for #2136

Signed-off-by: Abhishek Kumar [email protected]

Fixes #2068 aboutcode-org/skeleton#50 #2072

@Abhishek-Dev09-zz Abhishek-Dev09-zz force-pushed the #295-github_release branch 2 times, most recently from ee13ae6 to a1c0500 Compare July 16, 2020 18:16
It will upload all .whl,.ABOUT, .NOTICE,.LICENCE files of thirdparty repo to github new repo as an asset of specific OS and python 3 version and also generate archive for each Oses and all python and release them an asset on github.

Signed-off-by: Abhishek Kumar <[email protected]>
github_release.py Outdated Show resolved Hide resolved
Copy link
Contributor

@steven-esser steven-esser left a comment

Choose a reason for hiding this comment

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

Can we put these in etc/scripts/ like in #2118?

@Abhishek-Dev09-zz
Copy link
Author

Can we put these in etc/scripts/ like in #2118?

Yes, you can run from anywhere no problem

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Looking good!
I have a few nitpickings for your consideration

etc/scripts/deps_archive.py Outdated Show resolved Hide resolved
etc/scripts/github_release.py Outdated Show resolved Hide resolved
etc/scripts/github_release.py Show resolved Hide resolved
etc/scripts/github_release.py Outdated Show resolved Hide resolved
etc/scripts/github_release.py Outdated Show resolved Hide resolved
"""
Generate an archive for dependencies for specific OS and
given version of python by taking directory as an input.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me why and how this archive would be used?

Copy link
Author

Choose a reason for hiding this comment

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

It will generate an archive for each os and each python. For example - macOS_py36.tar.gz, win3_py36.zip, win64_py.36zip, linux_py36.tar.gz . For more info see PR description

Copy link
Member

Choose a reason for hiding this comment

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

I still do not know what these archives will be used for.... Are you saying that you plan to upload one such archive per OS/Python/scancode version to a GH release?
That archive will not be usable as a pip find-link then. And if there are dupes between OSes... that is going to be problematic. The archives we want to upload would be something built with etc/release/ scripts instead... e.g. full sdist with a streamlined thirparty

Copy link
Author

Choose a reason for hiding this comment

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

But you said earlier One archive for each OS/Python combo should be created and each should have its exact set of pinned requirements included in the built archive, no more and no less.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I would prefer to have the sdist being use for these archives that's using a proper manifest and does not include things we do not want there. For this you should expand on the release script instead

Copy link
Author

Choose a reason for hiding this comment

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

FYI , this will give you 1 directory along with tar.gz ,if you are on linux /macOS.
if you are on window, you get 1 directory along with zip.

BTW i got directory of wheels of 30.1 MB and tar.gz of 28 MB on my macOS.

etc/scripts/deps_archive.py Outdated Show resolved Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
from __future__ import print_function

import argparse
from commoncode.system import on_windows
Copy link
Member

Choose a reason for hiding this comment

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

Sort your imports: commoncode is not from the stdlib, but our own code and should go in a separate block.
See for instance https://github.com/nexB/scancode-toolkit/blob/develop/src/packagedcode/cargo.py#L36

deps_archive.py \\
--input thirdparty \\
--req requirements.txt
--output_file macOS_py36 \\
Copy link
Member

Choose a reason for hiding this comment

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

Please remove that as it does help readability.

root_dir = os.path.abspath(archive_name)
output_dir = os.path.abspath(archive_name)
if on_windows:
make_archive(output_dir, 'zip', root_dir)
Copy link
Member

Choose a reason for hiding this comment

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

I am stil not clear about what these archives wrt. to what we need as sdist

Copy link
Author

Choose a reason for hiding this comment

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

here you get all wheel/sdist deps packed like sdist file that required for ur python/os.

Signed-off-by: Philippe Ombredanne <[email protected]>
Do not spawn a process but call a function instead.
Clarify variable and options names and their help text.
Use access token from a cli option or an environment variable.

Signed-off-by: Philippe Ombredanne <[email protected]>
@pombredanne pombredanne force-pushed the #295-github_release branch from 2e6a01f to c062d21 Compare July 26, 2020 21:29
@@ -27,127 +27,138 @@
from __future__ import print_function

import argparse
from fnmatch import fnmatchcase
Copy link
Author

Choose a reason for hiding this comment

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

why you remove this?

from commoncode.fileutils import resource_iter

python_version = str(sys.version_info[0]) + str(sys.version_info[1])
Copy link
Author

Choose a reason for hiding this comment

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

Why remove this section ,it will help you filter the python 3 files if your directory contains multiple python deps.

or fnmatchcase(files, py_abi)
or (
fnmatchcase(files, '*tar.gz*')
and not fnmatchcase(files, '*py2-ipaddress-3.4.1.tar.gz*')
Copy link
Author

Choose a reason for hiding this comment

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

It will upload unwanted file also

'--body-string',
help='Required : Text describing the release. Ignored if the release already exists.',
'--token',
help=TOKEN_HELP,
Copy link
Author

Choose a reason for hiding this comment

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

great detail here.



def main_with_args(args: str) -> None:
api = grr.GithubApi(
Copy link
Author

Choose a reason for hiding this comment

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

Either you call API or cmd . result will be same.

type=str,
default='10',
required=False,
Copy link
Author

Choose a reason for hiding this comment

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

no need to mention that

type=str,
required=True,
required=False,
Copy link
Author

Choose a reason for hiding this comment

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

no need to mention, by default it is false



def main() -> None:
def main():
Copy link
Author

Choose a reason for hiding this comment

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

Why you remove anotations?

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

I am still not 100% clear about the deps archive and these will be used. But I am merging now so we can get a clearer picture and improve from that base.

@pombredanne pombredanne merged commit 07685a8 into aboutcode-org:develop Jul 27, 2020
@Abhishek-Dev09-zz Abhishek-Dev09-zz deleted the #295-github_release branch July 27, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants