OffTopic: DependencyUpdater

OffTopic: DependencyUpdater

[tlucas, 2017-11-06 14:15:13]

Hey guys!

I recently spoke to Dave in IRC about a script, which could handle the
automatable part of a dependency update.
I spent some of my free time on it (and dove into argparse and setuptools while
at it) and came up with this.

There are still some todos (mainly tests), but i wanted to put this thingy on
your radar.

If you have some extra 10-ish minutes after work and are eager to look at this,
there might be some things to discuss:

Is it worth it?
Do we officially want to publish this? (i wouldn't mind)
Where would we put this? (Codingtools?)

(For reference, the content of
https://issues.adblockplus.org/ticket/5757#comment:7 was rendered by this as a
test)


Cheers,
Tristan

https://codereview.adblockplus.org/29599579/diff/29599580/src/depup.py
File src/depup.py (right):

https://codereview.adblockplus.org/29599579/diff/29599580/src/depup.py#newcod...
src/depup.py:192: """
This is absolutely odd - but i didn't want to copy some code from
ensure_dependencies.py, neither did i want to sacrifice python3 compatibility,
so i decided to introduce this hack.

[kzar, 2017-11-06 14:32:27]

I'm really excited about the script Tristan and what I saw of it so far looked
really useful. I plan to play with it again soon, but have not had a chance.
(Also FYI this kind of stuff totally qualifies for your 20% time, so don't feel
like you have to do it in your own time.)

[tlucas, 2017-11-09 14:51:22]

I had some thoughts about it, maybe - in case this get's included / released -
the most appropriate place would be "codingtools".

Best,
Tristan

[kzar, 2017-11-10 14:32:02]

This already came in handy for #6022 thanks :) Sounds good to me having this in
the codingtools repository.

https://codereview.adblockplus.org/29599579/diff/29599580/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29599579/diff/29599580/.gitignore#newcode1
.gitignore:1: *.pyc
Maybe we need a .hgignore too if this is getting added to codingtools?

https://codereview.adblockplus.org/29599579/diff/29599580/README.md
File README.md (right):

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode3
README.md:3: ## Trivial
Nit: Maybe rename this to Introduction and add a paragraph at the start which
explains the rough idea about the tool and some of the things it can do?

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode5
README.md:5: Since there won't be any way to bridge submodules / subrepositories
between git and mercurial any time soon, we will stay dependent on a manual
approach for mirroring these ourselves.
Nit: "Git" and "Mercurial".

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode5
README.md:5: Since there won't be any way to bridge submodules / subrepositories
between git and mercurial any time soon, we will stay dependent on a manual
approach for mirroring these ourselves.
Nit: Mind wrapping some of these long lines where possible?

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode7
README.md:7: This program is meant to be pure convenience for those, who find
themselves in the need to update a project's dependency.
I guess this sentence can go when the above paragraph is added.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode33
README.md:33: 
Maybe add a note here that the revision for Mercurial will need to be added
manually if the local repository is Git and vice versa?

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode34
README.md:34: ## Running the programm
Would be nice to have an example which shows how to print a big diff of the
changes as well.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode53
README.md:53: $ depup issue adblockpluscore -r master -s
When I read this example I tried it without the `-r master -s` part and it
worked just the same. That made me wonder what those extra arguments did.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode54
README.md:54: 
Missing closing ```.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode98
README.md:98: ## Templating
Nit: IMO this section isn't too important, so should definitely be below the
Help section.

https://codereview.adblockplus.org/29599579/diff/29599580/setup.py
File setup.py (right):

https://codereview.adblockplus.org/29599579/diff/29599580/setup.py#newcode23
setup.py:23: packages=['src', ],
Nit: This comma inside the list literal seems redundant?

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
File src/templates/default.trac (right):

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
src/templates/default.trac:17: === Background ===
As mentioned in IRC it would be cool to print out the Issue subject at the top
here, then a blank line, then the issue body.

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
src/templates/default.trac:36: || {{ hg_hash }} || {{ git_hash }} ||
As mentioned in IRC I think the format like `hg:{{ hg_hash}} git:{{ git_hash }}`
would be more useful. (Or even better if the hashes were also links to the
commits in hg.adblockplus.org and Github too!)

[tlucas, 2017-11-13 14:34:09]

Patch Set 2
 * Addressing Dave's comments
 * Adding "raw_changes" to template context

https://codereview.adblockplus.org/29599579/diff/29599580/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29599579/diff/29599580/.gitignore#newcode1
.gitignore:1: *.pyc
On 2017/11/10 14:32:00, kzar wrote:
> Maybe we need a .hgignore too if this is getting added to codingtools?

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md
File README.md (right):

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode3
README.md:3: ## Trivial
On 2017/11/10 14:32:00, kzar wrote:
> Nit: Maybe rename this to Introduction and add a paragraph at the start which
> explains the rough idea about the tool and some of the things it can do?

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode5
README.md:5: Since there won't be any way to bridge submodules / subrepositories
between git and mercurial any time soon, we will stay dependent on a manual
approach for mirroring these ourselves.
On 2017/11/10 14:32:01, kzar wrote:
> Nit: Mind wrapping some of these long lines where possible?

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode7
README.md:7: This program is meant to be pure convenience for those, who find
themselves in the need to update a project's dependency.
On 2017/11/10 14:32:00, kzar wrote:
> I guess this sentence can go when the above paragraph is added.

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode33
README.md:33: 
On 2017/11/10 14:32:01, kzar wrote:
> Maybe add a note here that the revision for Mercurial will need to be added
> manually if the local repository is Git and vice versa?

As discussed ;)

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode34
README.md:34: ## Running the programm
On 2017/11/10 14:32:00, kzar wrote:
> Would be nice to have an example which shows how to print a big diff of the
> changes as well.

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode53
README.md:53: $ depup issue adblockpluscore -r master -s
On 2017/11/10 14:32:01, kzar wrote:
> When I read this example I tried it without the `-r master -s` part and it
> worked just the same. That made me wonder what those extra arguments did.

The title of the example should be sufficient imho (maybe because i'm the
author).
As a last instance, the help pages should definitely be the place to find all
about the parameters.

However, i slightly altered the title for all examples.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode54
README.md:54: 
On 2017/11/10 14:32:00, kzar wrote:
> Missing closing ```.

Not quite, the closing ``` are below the issue body.
However, i separated the command and the result.

https://codereview.adblockplus.org/29599579/diff/29599580/README.md#newcode98
README.md:98: ## Templating
On 2017/11/10 14:32:00, kzar wrote:
> Nit: IMO this section isn't too important, so should definitely be below the
> Help section.

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/setup.py
File setup.py (right):

https://codereview.adblockplus.org/29599579/diff/29599580/setup.py#newcode23
setup.py:23: packages=['src', ],
On 2017/11/10 14:32:01, kzar wrote:
> Nit: This comma inside the list literal seems redundant?

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
File src/templates/default.trac (right):

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
src/templates/default.trac:17: === Background ===
On 2017/11/10 14:32:01, kzar wrote:
> As mentioned in IRC it would be cool to print out the Issue subject at the top
> here, then a blank line, then the issue body.

Done.

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
src/templates/default.trac:36: || {{ hg_hash }} || {{ git_hash }} ||
On 2017/11/10 14:32:01, kzar wrote:
> As mentioned in IRC I think the format like `hg:{{ hg_hash}} git:{{ git_hash
}}`
> would be more useful. (Or even better if the hashes were also links to the
> commits in http://hg.adblockplus.org and Github too!)

I think this would useful, yes - but i personally wouldn't do that in the
default template (most dependency update issues i had a look at followed this
default pattern), i'd rather add another (fully functional) example to the
README.md, which would then include this.

[kzar, 2017-11-13 15:20:46]

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
File src/templates/default.trac (right):

https://codereview.adblockplus.org/29599579/diff/29599580/src/templates/defau...
src/templates/default.trac:36: || {{ hg_hash }} || {{ git_hash }} ||
On 2017/11/13 14:34:08, tlucas wrote:
> On 2017/11/10 14:32:01, kzar wrote:
> > As mentioned in IRC I think the format like `hg:{{ hg_hash}} git:{{ git_hash
> }}`
> > would be more useful. (Or even better if the hashes were also links to the
> > commits in http://hg.adblockplus.org and Github too!)
> 
> I think this would useful, yes - but i personally wouldn't do that in the
> default template (most dependency update issues i had a look at followed this
> default pattern), i'd rather add another (fully functional) example to the
> README.md, which would then include this.

I disagree on this one, but it's not hugely important.

https://codereview.adblockplus.org/29599579/diff/29606564/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29599579/diff/29606564/.hgignore#newcode1
.hgignore:1: syntax: glo
Typo, should be glob.

https://codereview.adblockplus.org/29599579/diff/29606564/README.md
File README.md (right):

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode7
README.md:7: This program is meant to ease up the process of gathering
information /
Nit: I think this reads better without the "up".

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode12
README.md:12: (represented by their commit messages), you can create a unified
diff of all
Nit: Typo "program".

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode12
README.md:12: (represented by their commit messages), you can create a unified
diff of all
Nit: I think this would read better if it were split into two sentences.
"...their commit messages). You can also create..."

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode69
README.md:69: ### Generate a bare issue body for updating to the local master
(-r master), but explicitly skip any mirroring (-s)
Nit: Might be clearer if you clarify what mirroring and master mean? "...local
master bookmark...", "...without mirroring Git if Mercurial is used (or vice
versa) to figure out the corresponding revision (-s)..."

[tlucas, 2017-11-20 15:24:43]

Patch Set 3:

 * Integrating into the codingtools folder-tree
 * Addressing Dave's comments on README.md
 * Adding a "commit" command (see new README.md)
 * Added tests for vcs.py
 * Fixed some regressions when using hg as the main repository

https://codereview.adblockplus.org/29599579/diff/29606564/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29599579/diff/29606564/.hgignore#newcode1
.hgignore:1: syntax: glo
On 2017/11/13 15:20:46, kzar wrote:
> Typo, should be glob.

Done.

https://codereview.adblockplus.org/29599579/diff/29606564/README.md
File README.md (right):

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode7
README.md:7: This program is meant to ease up the process of gathering
information /
On 2017/11/13 15:20:46, kzar wrote:
> Nit: I think this reads better without the "up".

Done.

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode12
README.md:12: (represented by their commit messages), you can create a unified
diff of all
On 2017/11/13 15:20:46, kzar wrote:
> Nit: I think this would read better if it were split into two sentences.
> "...their commit messages). You can also create..."

Done.

https://codereview.adblockplus.org/29599579/diff/29606564/README.md#newcode69
README.md:69: ### Generate a bare issue body for updating to the local master
(-r master), but explicitly skip any mirroring (-s)
On 2017/11/13 15:20:46, kzar wrote:
> Nit: Might be clearer if you clarify what mirroring and master mean? "...local
> master bookmark...", "...without mirroring Git if Mercurial is used (or vice
> versa) to figure out the corresponding revision (-s)..."

Nice idea, done.

[tlucas, 2017-11-27 09:45:05]

Patch Set 3:

 * Removed obsolete text in 'issue' help page
 * Execute VCS to determin repository type
 * Let flake8 be it's own tox environment / command

[kzar, 2017-11-27 15:03:35]

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/README.md
File eyeo-depup/README.md (right):

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/README.m...
eyeo-depup/README.md:6: preparing an issue on issues.adblockplus.org, in order
to update a given
Maybe remove "on issues.adblockplus.org"?

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/README.m...
eyeo-depup/README.md:9: You can run this program to get a list of potentially
included changes
Maybe reword this to "You can run this program to get a list of included
commits, view a unified diff of those changes and to generate a template issue
for our issue tracker. After you have filed the issue (and have a issue number)
you can use this program to commit the dependency update itself ready for code
review."

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/README.m...
eyeo-depup/README.md:16: Since there won't be any way to bridge submodules /
subrepositories between
Maybe add more context here? "In order to provide commit hashes for both
Mercurial and Git..."

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/README.m...
eyeo-depup/README.md:143: ...
Nit: Duplicate ellipses.

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
File eyeo-depup/src/templates/default.trac (right):

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
eyeo-depup/src/templates/default.trac:19: Update {{ repository }} dependency to
{{ hg_hash }}
Nit: Mind removing "SUBJECT: " and changing adding "revision"?

"Update {{ repository }} dependency to revision { hg_hash }}"

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
eyeo-depup/src/templates/default.trac:22: 
Nit: This blank line is inconsistent with the other sections.

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
eyeo-depup/src/templates/default.trac:43: 
Nit: Another extra blank line.

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
eyeo-depup/src/templates/default.trac:47: 
Nit: Another extra blank line.

[kzar, 2017-11-28 11:39:32]

I just tried piping the output of depup diff to the `more` command or into a
file, for both I receive this error:

WARNING: you are trying to downgrade the dependency!
Traceback (most recent call last):
  File "/usr/local/bin/depup", line 4, in <module>
    __import__('pkg_resources').run_script('depup==0.1', 'depup')
  File "/usr/lib/python2.7/dist-packages/pkg_resources/__init__.py", line 750,
in run_script
    self.require(requires)[0].run_script(script_name, ns)
  File "/usr/lib/python2.7/dist-packages/pkg_resources/__init__.py", line 1524,
in run_script
    exec(code, namespace, namespace)
  File
"/usr/local/lib/python2.7/dist-packages/depup-0.1-py2.7.egg/EGG-INFO/scripts/depup",
line 25, in <module>
    DepUpdate()()
  File
"/usr/local/lib/python2.7/dist-packages/depup-0.1-py2.7.egg/src/depup.py", line
359, in __call__
    output = action_map[self.arguments.action]()
  File
"/usr/local/lib/python2.7/dist-packages/depup-0.1-py2.7.egg/src/depup.py", line
255, in build_diff
    self.arguments.unified_lines))
UnicodeEncodeError: 'ascii' codec can't encode characters in position
20150-20152: ordinal not in range(128)

[Sebastian Noack, 2018-06-01 17:46:43]

FYI, when I install the thing with "pip install ." (rather than "python setup.py
install), the "issues" command fails with "jinja2.exceptions.TemplateNotFound:
default.trac".

[Sebastian Noack, 2018-06-01 18:08:16]

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
File eyeo-depup/src/templates/default.trac (right):

https://codereview.adblockplus.org/29599579/diff/29620555/eyeo-depup/src/temp...
eyeo-depup/src/templates/default.trac:41: 
In case of a buildtools dependency update you also have to run
./ensure_depedencies.py (in order to update the contents of
ensure_depedencies.py if there were any changes). I wonder whether we should add
something like this here:

  {% if repository == 'buildtools' -%}
  Run `./ensure_depedencies.py`.
  {%- endif %}

[Sebastian Noack, 2018-06-01 18:38:15]

FWIW, a similar use case: Every time before a release I manually double check
that all issues referenced in commits between the last release and the current
master are closed and assigned to the current milestone, and the other way
around that all closed issues in the current milestone have a commit referencing
them. Perhaps we can add a command to dedup to automate that as well?