Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(14)

Issue 5168251361296384: Issue 170 - Replacing Mercurial subrepositories (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by Wladimir Palant
Modified:
5 years ago
Visibility:
Public.

Description

See issue 170 for the specification. This implementation is mostly done, the Git pull command is currently wrong however. Also, the self-update mechanism isn`t implemented yet.

Patch Set 1 #

Total comments: 24

Patch Set 2 : Addressed comments and fixed git pull command #

Patch Set 3 : Added self-update #

Patch Set 4 : Renamed script #

Total comments: 13

Patch Set 5 : Addressed comments #

Total comments: 3

Patch Set 6 : Removed useless exit() call #

Patch Set 7 : Do not reload when used as module #

Total comments: 6

Patch Set 8 : Better safe_join(), usage info, additional parameters #

Total comments: 4

Patch Set 9 : Addressed remaining comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+255 lines, -0 lines) Patch
A ensure_dependencies.py View 1 2 3 4 5 6 7 8 1 chunk +255 lines, -0 lines 0 comments Download

Messages

Total messages: 32
Wladimir Palant
5 years ago (2014-09-01 20:14:49 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ensureDeps.py:28: class Mercurial(): Since Mercurial and Git only have static ...
5 years ago (2014-09-02 08:12:30 UTC) #2
Wladimir Palant
Current patch is still work in progress but now only the self-update mechanism is missing. ...
5 years ago (2014-09-02 14:48:03 UTC) #3
Felix Dahlke
Some remarks: 1. I think we should call the script ensuredeps.py or ensure_deps.py. PEP8 says ...
5 years ago (2014-09-03 12:30:16 UTC) #4
Wladimir Palant
As discussed on IRC, the problem with .deps is a potential naming conflict with automake ...
5 years ago (2014-09-03 13:12:21 UTC) #5
Sebastian Noack
I would also prefer a filename without camel case in regard to PEP-8. http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ...
5 years ago (2014-09-03 19:23:07 UTC) #6
Wladimir Palant
Self-update mechanism has been added, the functionality should be complete now. Note that you should ...
5 years ago (2014-09-03 21:57:47 UTC) #7
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode45 ensureDeps.py:45: result, dummy = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate() On 2014/09/03 21:57:47, ...
5 years ago (2014-09-04 10:07:29 UTC) #8
Wladimir Palant
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode89 ensureDeps.py:89: print >>sys.stderr, "Invalid line in file %s: %s" % ...
5 years ago (2014-09-04 13:32:12 UTC) #9
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py#newcode218 ensure_dependencies.py:218: import importlib On 2014/09/04 13:32:12, Wladimir Palant wrote: > ...
5 years ago (2014-09-04 13:44:52 UTC) #10
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py#newcode216 ensure_dependencies.py:216: os.execv(sys.executable, [sys.executable, target] + sys.argv[1:]) On 2014/09/04 10:07:30, Sebastian ...
5 years ago (2014-09-04 13:50:17 UTC) #11
Wladimir Palant
http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py#newcode218 ensure_dependencies.py:218: import importlib On 2014/09/04 13:44:52, Sebastian Noack wrote: > ...
5 years ago (2014-09-04 14:34:45 UTC) #12
Wladimir Palant
http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py#newcode218 ensure_dependencies.py:218: import importlib Actually, this doesn't seem to be that ...
5 years ago (2014-09-04 15:55:37 UTC) #13
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5754903989321728/ensure_dependencies.py#newcode218 ensure_dependencies.py:218: import importlib On 2014/09/04 15:55:38, Wladimir Palant wrote: > ...
5 years ago (2014-09-04 16:22:00 UTC) #14
Wladimir Palant
The new version will now advise a manual restart if ensure_dependencies.py is used as a ...
5 years ago (2014-09-04 17:18:17 UTC) #15
Sebastian Noack
LGTM
5 years ago (2014-09-05 08:34:24 UTC) #16
Felix Dahlke
I think it would be neat to have a small example dependencies file on top, ...
5 years ago (2014-09-05 08:55:52 UTC) #17
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ensureDeps.py:28: class Mercurial(): On 2014/09/05 08:55:53, Felix H. Dahlke wrote: ...
5 years ago (2014-09-05 09:29:30 UTC) #18
Felix Dahlke
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ensureDeps.py:28: class Mercurial(): On 2014/09/05 09:29:30, Sebastian Noack wrote: > ...
5 years ago (2014-09-05 09:55:01 UTC) #19
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ensureDeps.py:28: class Mercurial(): On 2014/09/05 09:55:01, Felix H. Dahlke wrote: ...
5 years ago (2014-09-05 10:47:05 UTC) #20
Felix Dahlke
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ensureDeps.py:28: class Mercurial(): On 2014/09/05 10:47:06, Sebastian Noack wrote: > ...
5 years ago (2014-09-05 14:01:53 UTC) #21
Sebastian Noack
http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py File ensureDeps.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ensureDeps.py:28: class Mercurial(): On 2014/09/05 14:01:54, Felix H. Dahlke wrote: ...
5 years ago (2014-09-05 14:24:46 UTC) #22
Felix Dahlke
On 2014/09/05 14:24:46, Sebastian Noack wrote: > http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py > File ensureDeps.py (right): > > http://codereview.adblockplus.org/5168251361296384/diff/5629499534213120/ensureDeps.py#newcode28 ...
5 years ago (2014-09-05 17:05:39 UTC) #23
Sebastian Noack
On 2014/09/05 17:05:39, Felix H. Dahlke wrote: > On 2014/09/05 14:24:46, Sebastian Noack wrote: > ...
5 years ago (2014-09-05 17:10:18 UTC) #24
Felix Dahlke
On 2014/09/05 17:10:18, Sebastian Noack wrote: > On 2014/09/05 17:05:39, Felix H. Dahlke wrote: > ...
5 years ago (2014-09-05 17:36:18 UTC) #25
Wladimir Palant
In addition to addressing the comments, the new patch set added a bunch of parameters ...
5 years ago (2014-09-05 18:37:24 UTC) #26
Wladimir Palant
http://codereview.adblockplus.org/5168251361296384/diff/5641332169113600/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5641332169113600/ensure_dependencies.py#newcode127 ensure_dependencies.py:127: return os.path.join(path, *[f for f in subpath.split("/") if f ...
5 years ago (2014-09-05 18:37:39 UTC) #27
Sebastian Noack
Still LGTM.
5 years ago (2014-09-06 15:07:42 UTC) #28
Felix Dahlke
Looks pretty good now, just some minor suggestions. http://codereview.adblockplus.org/5168251361296384/diff/5735735550279680/ensure_dependencies.py File ensure_dependencies.py (right): http://codereview.adblockplus.org/5168251361296384/diff/5735735550279680/ensure_dependencies.py#newcode30 ensure_dependencies.py:30: usage ...
5 years ago (2014-09-06 20:48:32 UTC) #29
Felix Dahlke
On 2014/09/06 20:48:32, Felix H. Dahlke wrote: > Looks pretty good now, just some minor ...
5 years ago (2014-09-09 06:41:24 UTC) #30
Wladimir Palant
@Sebastian: Felix is on vacation so it is up to you to verify that I ...
5 years ago (2014-09-09 15:47:36 UTC) #31
Sebastian Noack
5 years ago (2014-09-09 17:30:29 UTC) #32
Still LGTM.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5