Recursive Packaging

[Korusuke]

This is by no means complete, but wanted to share and get input on whether is it right and what else can we do.

For this to work changes required on upt side will be this:

packages = list(args.package)
i = 0
while i < len(packages):
      try:
           upt_pkg = None
           upt.log.logger_set_formatter(logger, 'Frontend')
           upt_pkg = frontend.parse(packages[i])
           upt_pkg.frontend = args.frontend
           upt.log.logger_set_formatter(logger, 'Backend')
           upt_pkg.packages = list(packages)
           upt_pkg = backend.create_package(upt_pkg, output=args.output)
           packages = list(upt_pkg.packages)
           i += 1

So currently as you can see, the code doesn't check MacPorts Tree and neither ask the user. I tested this out for upt-cpan and it generated this list:

['upt-cpan', 'requests', 'upt', 'requests-mock', 'chardet', 'idna', 
'urllib3', 'certifi', 'spdx-lookup', 'six', 'fixtures', 'mock', 
'purl', 'pytest', 'sphinx', 'testrepository', 'testtools', 'hypothesis',
 'pbr', 'pytest-cov', 'py', 'packaging', 'attrs', 'atomicwrites', 'pluggy', 
 'importlib-metadata', 'wcwidth', 'argcomplete', 'nose', 'sphinxcontrib-applehelp', 
 'sphinxcontrib-devhelp', 'sphinxcontrib-jsmath', 'sphinxcontrib-htmlhelp', 
 'sphinxcontrib-serializinghtml', 'sphinxcontrib-qthelp', 'Jinja2', 'Pygments', 
 'docutils', 'snowballstemmer', 'babel', 'alabaster', 'imagesize', 'setuptools', 
 'html5lib', 'flake8', 'flake8-import-order', 'mypy', 'docutils-stubs', 'extras', 
 'python-mimeparse', 'traceback2', 'unittest2', 'coverage', 'fields', 'hunter', 
 'process-tests', 'virtualenv', 'pyparsing', 'pympler', 'zope.interface', 'zipp', 
 'pexpect', 'wheel', 'MarkupSafe', 'pytz', 'webencodings', 'entrypoints', 'pyflakes', 
 'pycodestyle', 'mccabe', 'typed-ast', 'mypy-extensions', 'linecache2', 'contextlib2', 
 'argparse', 'colorama', 'pytest-timeout', 'pytest-xdist', 'pytest-localserver', 
 'pypiserver', 'zope.event', 'pathlib2', 'ptyprocess', 'execnet', 'pytest-forked', 
 'filelock', 'zope.testrunner', 'apipkg', 'zope.exceptions', 'zope.testing']

Which as you can see is very big and my guess is not required for just upt-cpan. If we just take the run dependencies then the output is similar to what pypi would install, which is:

['upt-cpan', 'requests', 'upt', 'chardet', 'idna', 'urllib3', 'certifi', 'spdx-lookup']

So if the way I am doing it right now is fine then next thing would be to add MacPorts tree check which can be done directly by "guessing" the dependency name and a get request like for upt to this url https://raw.githubusercontent.com/macports/macports-ports/master/python/py-upt/Portfile

Is there any better way to search github for package name in macports tree so that we don't have to "guess" the path and/or can we directly get the version of the package from MacPorts somehow.

and then comes version extraction and decision that whether a Portfile should be generated or not.

In this I am not sure if we should do this fully automatically, here we can keep user-input.

[Steap]

Which as you can see is very big and my guess is not required for just upt-cpan.

I don't understand how you came to that conclusion. If this is the list you computed, then it means that they are actually required.

If we just take the run dependencies then the output is similar to what pypi would install, which is:

['upt-cpan', 'requests', 'upt', 'chardet', 'idna', 'urllib3', 'certifi', 'spdx-lookup']

But we probably do not want that. Why would we drop the test dependencies?

[Korusuke]

Which as you can see is very big and my guess is not required for just upt-cpan.

I don't understand how you came to that conclusion. If this is the list you computed, then it means that they are actually required.

If we just take the run dependencies then the output is similar to what pypi would install, which is: ['upt-cpan', 'requests', 'upt', 'chardet', 'idna', 'urllib3', 'certifi', 'spdx-lookup']

But we probably do not want that. Why would we drop the test dependencies?

Sorry, I did not mean that they are not required but even while installing via MacPorts currently not all of this ports are installed. But by this, I am not implying that they are not required but maybe it works without them.

Other than this is it fine the way I am implementing the feature?

[Korusuke]

So as suggested by @reneeotten in #32 to use port info <pkg-name>. I have added a preliminary version of that.

For version compatibility checking, I am not sure how would we do that as when we are adding a specific port to the generate list at that point we don't know the upstream version and only know the MacPorts version and the acceptable version for the parent pkg.

One possible solution which is a bit messy is to add the macports version as a property to the list and when we are to generate the package for that we compare the upstream version with the macports version and acceptable range, but this makes the code a bit hard to read.

As of now it is recursively packing only if the port is not available in the MacPorts tree without any version checking.

Another query I had is should we use port info as that make upt-macports os dependent. All though a bit tricky but we can use github as suggested in the above comment.

[Korusuke]

With this changes done, I would say the flow although a bit weird is good enough. Only thing left is to add the constraint checking and then it would be ready to merge. But it can be merged before that too as constraint checking may not be always reliable.

Current output for upt package -f pypi -b macport upt --recursive is:

[INFO    ] [Backend] Creating MacPorts package for upt
[INFO    ] [Backend] Checking MaPorts tree for port py-upt
[INFO    ] [Backend] Current MacPorts Version for py-upt is 0.7
[INFO    ] [Backend] Current Upstream version is 0.9
[INFO    ] [Backend] Creating Portfile as new version is available
[INFO    ] [Backend] Creating MacPorts package for spdx-lookup
[INFO    ] [Backend] Checking MaPorts tree for port py-spdx-lookup
[INFO    ] [Backend] Current MacPorts Version for py-spdx-lookup is 0.3.2
[INFO    ] [Backend] Current Upstream version is 0.3.2
[INFO    ] [Backend] Skipping spdx-lookup as MacPorts Portfile is already up-to-date

Any changes or review on the overall flow or the code itself ?

[Steap]

Please show the upt code by sending a pull request to https://framagit.org/upt/upt .

[INFO ] [Backend] Checking MaPorts tree for port py-upt [INFO ] [Backend] Current MacPorts Version for py-upt is 0.7 [INFO ] [Backend] Current Upstream version is 0.9

Looks like you're updating ports without the --update flag.

[Steap]

Also, there is a lot of logic here that should be part of upt itself.

[Korusuke]

Updated the existing pull request, will squash the commits once everything is finalized :)

I am aware that some of the logic could be transferred to upt itself but I was avoiding that as much as possible as any major change in upt would mean that all other backends would have to be updated too. But please do point out anything that you would like to be moved to upt itself.

[Korusuke]

Please show the upt code by sending a pull request to https://framagit.org/upt/upt .

[INFO ] [Backend] Checking MaPorts tree for port py-upt [INFO ] [Backend] Current MacPorts Version for py-upt is 0.7 [INFO ] [Backend] Current Upstream version is 0.9

Looks like you're updating ports without the --update flag.

No, I am not updating the ports. It is just comparing the version and if new version is available than a new Portfile is created without using any data from the old Portfile. Whereas in update feature a custom parser for MacPorts portfile will be required to update only the necessary fields.

[Steap]

I am aware that some of the logic could be transferred to upt itself but I was avoiding that as much as possible as any major change in upt would mean that all other backends would have to be updated too. But please do point out anything that you would like to be moved to upt itself.

Not necessarily. If a backend does not implement the "recursive" feature, it just means its users won't be able to use "--recursive", end of story.

The point of upt is to put everything complex in upt itself, and have easy-to-write modules. Think of the current state of the backends: a backend is just a few templates and some glue code that takes care of simple things (such as naming conventions) that are specific to this backend.

The only thing required on the backend side should be a "do you have this package already?" method. The complex code should be in upt itself and look something like (in pseudo-code):

function do_package(pkgname):
    pkg = frontend.parse(pkgname)
    backend.create_package(pkg)
    if !recursive:
        return
    for dep in pkg.all_dependencies():
        if backend.has_package(dep):
            continue
        do_package(dep)

Obviously we will need logging, error handling, etc. But first, we should get something like this to work. The initial implementation of has_package() could be "return True". Once this works, we can add special cases and various enhancements.

[Korusuke]

Updated both the PR's, upt is currently failing on one test will fix it if the code changes looks good. I am not so happy with how the upt code looks mainly because of no. of arguments in pkg_it().

Now it almost follows the way @Steap suggested except that has_package() is replaced by check_needed() which also compares the version and can have more logic.

Yes we can have the version comparison logic in upt but I would say that different backends may decide differently based on versions and other parameters like in ruby-gems some backends wont consider the configure phase dependencies.

I tested the current PR with upt and upt-cpan as root packages and it works fine. Now it is Depth First Traversal unlike before.

[reneeotten]

Another query I had is should we use port info as that make upt-macports os dependent. All though a bit tricky but we can use github as suggested in the above comment.

You are right that the use of port to check whether a port is already present in the MacPorts tree and it's version information would require MacPorts to be installed. I am totally fine with that, you have recently installed MacPorts on Linux so it could still work there; I think that we can expect that anyone wanting to package something for MacPorts can actually run the port command.

[Korusuke]

Tests added :)

[Korusuke]

Anything else that needs to be modified in this or corresponding upt PR ?

[codecov[bot]]

Codecov Report

Merging #35 into master will increase coverage by 0.86%. The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   85.62%   86.48%   +0.86%     
==========================================
  Files           1        1              
  Lines         160      185      +25     
  Branches        7       10       +3     
==========================================
+ Hits          137      160      +23     
- Misses         23       25       +2

Impacted Files	Coverage Δ
upt_macports/upt_macports.py	86.48% <89.28%> (+0.86%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 32e67bd...8747aa6. Read the comment docs.

[Korusuke]

I guess we can merge this now as @Steap has already added Recursive feature in upt core.

[reneeotten]

I guess we can merge this now as @Steap has already added Recursive feature in upt core.

I will install the latest upt and the code from this PR and give it a last test today/tomorrow; if everything works as intended it can indeed be merged.

[Korusuke]

While writing the update feature, there are a few things that I guess will be changed in package_versions() so please don't merge this as I would like to change those here itself...will commit those changes tomorrow once I am sure of it.

[reneeotten]

okay, I did some testing and to from a user-perspective it all seems to work as intended - very cool!

I am not the biggest fan of the messages for checking whether to package something or not; in MacPorts there is always only one version available. But, I understand that this is needed in upt for other backends and I don't think it's worthwhile to override the def needs_requirement(self, req, phase): just for some more accurate or (for me personally) more pleasing messages ;)

[INFO ] [Backend] Dependency numpy (>=1.0): at least one compatible version found in ['1.16.4']. Not packaging it.

There are two "unresolved" (related) conversation that were brought up by @Steap, so let's try to fix that. We'll wait for what other changes you think are needed before merging. Again, please do NOT force-push to this branch, just add separate commits for now; it makes it easier to review the changes and if needed we can squash things before merging the PR.

[Steap]

I hadn't thought of that :) It is true that it feels unnatural to have a list for MacPorts, but I guess it's a minor inconvenience :)

I agree with the idea of squashing. It's too bad github cannot remember multiple commits like Gerrit does, for instance.

[Steap]

Commit "Not scanning for frontend when creating output dir " should probably be a separate PR. It would be fairly simple to review and merge, and would in turn make this PR easier to review.

[Korusuke]

Commit "Not scanning for frontend when creating output dir " should probably be a separate PR. It would be fairly simple to review and merge, and would in turn make this PR easier to review.

Moved to another PR

[Steap]

So this patch looks quite good now.

@reneeotten What do you think, especially about the MacPorts-specific parts?

Should @Korusuke squash the commits, rebase them (if needed) and show us a "final" patch?

[reneeotten]

So this patch looks quite good now.

@reneeotten What do you think, especially about the MacPorts-specific parts?

Should @Korusuke squash the commits, rebase them (if needed) and show us a "final" patch?

It looks good to me; just tested a little bit again and it works as intended. @Korusuke please change the indentation and squash the commits as @Steap suggested and then let's merge this!

[reneeotten]

@Steap I think we should merge this and will do so in a few hours unless you have objections. If you agree, feel free to merge it yourself at your earliest convenience ;)