macports / upt-macports

The Universal Packaging Tool (upt) backend for MacPorts
https://framagit.org/upt/upt
BSD 3-Clause "New" or "Revised" License
8 stars 12 forks source link

Set homepage in templates when needed #64

Closed reneeotten closed 4 years ago

reneeotten commented 4 years ago

If upstream has set a homepage, and that one is different than what is set by the perl5-1.0 PG, add it to the template. Otherwise let the PG take care of it.

codecov[bot] commented 4 years ago

Codecov Report

Merging #64 into master will increase coverage by 0.32%. The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   90.67%   90.99%   +0.32%     
==========================================
  Files           1        1              
  Lines         193      211      +18     
  Branches       15       18       +3     
==========================================
+ Hits          175      192      +17     
- Misses         16       17       +1     
  Partials        2        2
Impacted Files Coverage Δ
upt_macports/upt_macports.py 90.99% <94.44%> (+0.32%) :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 63a7072...79c9c51. Read the comment docs.

Steap commented 4 years ago

I know I wrote some "complex things" in the templates (such as the "depends" macro), but I usually like to keep the templates simple for two reasons:

So I would rather do something like this:

class MacportsPackage(object):
    ...
    @property
    def homepage(self):
        return self.upt_pkg.homepage

class MacPortsPerlPackage(MacPortsPackage):
    ...
    @property
    def homepage(self):
        if foo:
            return 'http://...'
        else:
            return None

In the template we would write:

{% if pkg.homepage %}
homepage            {{ pkg.homepage }}
{% endif %}

We could then fix the Rubygems issue the same way, and have a bunch of unit tests for everything.

reneeotten commented 4 years ago

@Steap I see your point and do think it makes sense to do some of this in the backend instead of templates.

In fact, I started to add a function to automtically "convert" upstream homepages to https if we know that the hosts support them (e.g., github.com, rubygems.org, etcetera). That is indeed in the MacportsPackage already, so let me try and combine the PRs with homepage-related changes into one.

Steap commented 4 years ago

Do you have examples of packages that report "http" instead of "https"? I think this change should make its way to the frontends, as all backends would probably like that.

reneeotten commented 4 years ago

okay, I will update this PR to do the homepage conversion stuff in the backend and simplify the templates.

Do you have examples of packages that report "http" instead of "https"? I think this change should make its way to the frontends, as all backends would probably like that.

I agree, I think it would actually make sense to add this to upt itself as not to duplicate the code in every frontend. Something along the lines I added to upt_macports would probably work:

    def _homepage(self):
        upstream_homepage = self.upt_pkg.homepage

        if upstream_homepage.startswith('https'):
            return upstream_homepage

        https_hosts = ('github', 'readthedocs', 'pypi', 'bitbucket',
                       'metacpan', 'rubygems')
        homepage_split = upstream_homepage.lstrip('http://').split('.')

        if set(homepage_split).intersection(https_hosts):
            return upstream_homepage.replace('http', 'https')
        else:
            return upstream_homepage

Below are a few examples of ports for which upstream is using http, but that could use https.

Steap commented 4 years ago

I went for a slightly different implementation, what do you think about https://framagit.org/upt/upt/commit/5409ff3a78b78eb526a109b15df3b9ae8c6d5028 ?

reneeotten commented 4 years ago

I went for a slightly different implementation, what do you think about https://framagit.org/upt/upt/commit/5409ff3a78b78eb526a109b15df3b9ae8c6d5028 ?

Yes, your way of checking if one can use https is much more general and easier to maintain than my poor-man's-approach of adding "known, good hosts that support https". Looks good to me!

I am flying out for the holidays in a few hours, but will eventually update this PR with a more generalized solution for setting the homepage.

reneeotten commented 4 years ago

@Steap how about this?

reneeotten commented 4 years ago

@Steap any additional comments here? If not, I would like to merge this. Of course it would be helpful to add a default in upt-pypi and such; once that has happened we can always update the code in upt-macports accordingly. Also, I will look into the URL you suggest for CPAN packages; if we go for that it will need to be changed in the perl PortGroup as well. Again we can defer this to a later time I think.

reneeotten commented 4 years ago

okay, I am going to merge this now - we can make adjustments if things change in upt or the frontends.