Closed Bug 1275409 Opened 4 years ago Closed 4 years ago

Delete unused code from testing/taskcluster

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla50

People

(Reporter: dustin, Assigned: dustin)

References

Details

Attachments

(15 files)

58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
58 bytes, text/x-review-board-request
wcosta
: review+
Details
3.79 KB, patch
mshal
: review+
Details | Diff | Splinter Review
Most of the code in testing/taskcluster/taskcluster_graph is now unused, with taskcluster/taskgraph replacing it.  There's some imports of the former from the latter.

This should get cleaned up so that we don't have dead code hanging around in the tree.
This should include "rescuing" a few of the './mach taskcluster_foo` utility commands, implementing them in the new directory.
This also includes syncing up the alias lists between the YML and try_option_parser.py
Code for this is ready in my branch, just waiting for bug 1274611 to land.
Duplicate of this bug: 1277594
Review commit: https://reviewboard.mozilla.org/r/58026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58026/
Attachment #8760480 - Flags: review?(wcosta)
Attachment #8760481 - Flags: review?(gps)
Attachment #8760482 - Flags: review?(wcosta)
Attachment #8760483 - Flags: review?(wcosta)
Attachment #8760484 - Flags: review?(wcosta)
Attachment #8760485 - Flags: review?(wcosta)
Attachment #8760486 - Flags: review?(wcosta)
Attachment #8760487 - Flags: review?(wcosta)
Attachment #8760488 - Flags: review?(wcosta)
Attachment #8760489 - Flags: review?(wcosta)
Attachment #8760490 - Flags: review?(wcosta)
Attachment #8760491 - Flags: review?(wcosta)
Attachment #8760492 - Flags: review?(wcosta)
Attachment #8760493 - Flags: review?(wcosta)
Attachment #8760481 - Flags: review?(gps)
Comment on attachment 8760481 [details]
Bug 1275409: mock os.path.is{dir,file} too;

https://reviewboard.mozilla.org/r/58028/#review54938

::: config/mozunit.py:182
(Diff revision 1)
>              return True
>  
> +        return self._orig_path_isfile(p)
> +
> +    def _wrapped_isdir(self, p):
> +        p = p if p.endswith('/') else p + '/'

This assumes POSIX paths. You'll want to test for `endswith(('/', '\\'))` to catch Windows.

You can continue to use `/` for constructing paths on Windows because Windows accepts both forward and back slashes in its APIs.
Comment on attachment 8760481 [details]
Bug 1275409: mock os.path.is{dir,file} too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58028/diff/1-2/
Attachment #8760481 - Flags: review?(gps)
Comment on attachment 8760482 [details]
Bug 1275409: move templates to taskgraph.util;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58030/diff/1-2/
Comment on attachment 8760483 [details]
Bug 1275409: move taskcluster_graph.from_now to taskgraph.util.time;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58032/diff/1-2/
Comment on attachment 8760484 [details]
Bug 1275409: remove taskcluster_graph.transform;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58034/diff/1-2/
Comment on attachment 8760485 [details]
Bug 1275409: remove taskcluster_graph.image_builder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58036/diff/1-2/
Comment on attachment 8760486 [details]
Bug 1275409: remove mach_util;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58038/diff/1-2/
Comment on attachment 8760487 [details]
Bug 1275409: move taskcluster_graph.commit_parser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58040/diff/1-2/
Comment on attachment 8760488 [details]
Bug 1275409: remove taskcluster_graph.build_task;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58042/diff/1-2/
Comment on attachment 8760489 [details]
Bug 1275409: remove unused aliases list in yaml files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58044/diff/1-2/
Comment on attachment 8760490 [details]
Bug 1275409: remove remaining code in testing/taskcluster;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58046/diff/1-2/
Comment on attachment 8760491 [details]
Bug 1275409: remove unused legacy task definitions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58048/diff/1-2/
Comment on attachment 8760492 [details]
Bug 1275409: move legacy task definitions into legacy kind;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58050/diff/1-2/
Comment on attachment 8760493 [details]
Bug 1275409: move legacy taskcluster-related scripts to taskcluster/scripts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58052/diff/1-2/
Comment on attachment 8760481 [details]
Bug 1275409: mock os.path.is{dir,file} too;

https://reviewboard.mozilla.org/r/58028/#review55120
Attachment #8760481 - Flags: review?(gps) → review+
Attachment #8760480 - Flags: review?(wcosta) → review+
Comment on attachment 8760480 [details]
Bug 1275409: remove testing/taskcluster/mach_commands.py, except 'mach taskcluster-load-image';

https://reviewboard.mozilla.org/r/58026/#review55178

::: taskcluster/mach_commands.py:216
(Diff revision 1)
> +            sys.exit(1)
> +
> +        if not task_id:
> +            task_id = task_id_for_image({}, 'mozilla-inbound', image_name, create=False)
> +            if not task_id:
> +                print("No task found in the TaskCluster index for {}".format(image_name))

Simpler if you do
print("No task found in the TaskCluster index for", image_name)

::: taskcluster/mach_commands.py:219
(Diff revision 1)
> +            task_id = task_id_for_image({}, 'mozilla-inbound', image_name, create=False)
> +            if not task_id:
> +                print("No task found in the TaskCluster index for {}".format(image_name))
> +                sys.exit(1)
> +
> +        print("Task ID: {}".format(task_id))

print("Task ID:", task_id)

::: taskcluster/mach_commands.py:224
(Diff revision 1)
> +        print("Task ID: {}".format(task_id))
> +
> +        ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{}/artifacts/{}'
> +        image_name = docker_load_from_url(ARTIFACT_URL.format(task_id, 'public/image.tar'))
> +
> +        print("Loaded image is named {}".format(image_name))

print("Loaded image is named", image_name)
Comment on attachment 8760482 [details]
Bug 1275409: move templates to taskgraph.util;

https://reviewboard.mozilla.org/r/58030/#review55184

::: taskcluster/docs/yaml-templates.rst:9
(Diff revision 2)
> +Many kinds of tasks are described using YAML files.  These files allow some
> +limited forms of inheritance and template substitution as well as the usual
> +YAML features, as described below.
> +
> +Please use these features sparingly.  In many cases, it is better to add a
> +feature to the implementation of a task kind than to add complexity to the YAML

s/than to add/rather than adding/ ?

::: taskcluster/taskgraph/util/docker.py:13
(Diff revision 2)
> +
> +GECKO = os.path.realpath(os.path.join(__file__, '..', '..', '..', '..'))
> +DOCKER_ROOT = os.path.join(GECKO, 'testing', 'docker')
> +
> +def docker_image(name):
> +    ''' Determine the docker image name, including repository and tag, from an

nit: remove space after '''
Attachment #8760482 - Flags: review?(wcosta) → review+
Comment on attachment 8760483 [details]
Bug 1275409: move taskcluster_graph.from_now to taskgraph.util.time;

https://reviewboard.mozilla.org/r/58032/#review55188
Attachment #8760483 - Flags: review?(wcosta) → review+
Attachment #8760484 - Flags: review?(wcosta) → review+
Comment on attachment 8760484 [details]
Bug 1275409: remove taskcluster_graph.transform;

https://reviewboard.mozilla.org/r/58034/#review55190

::: taskcluster/taskgraph/kind/legacy.py:89
(Diff revision 2)
> +    task['extra']['treeherder']['revision'] = revision
> +    task['extra']['treeherder']['revision_hash'] = revision_hash
> +
> +
> +def decorate_task_treeherder_routes(task, suffix):
> +    """

nit: doc string should start at the same line as """

::: taskcluster/taskgraph/kind/legacy.py:111
(Diff revision 2)
> +
> +    for env in treeheder_env:
> +        task['routes'].append('{}.{}'.format(TREEHERDER_ROUTES[env], suffix))
> +
> +def decorate_task_json_routes(task, json_routes, parameters):
> +    """

nit: doc string should start at the same line as """
Comment on attachment 8760485 [details]
Bug 1275409: remove taskcluster_graph.image_builder;

https://reviewboard.mozilla.org/r/58036/#review55194

::: taskcluster/mach_commands.py:208
(Diff revision 2)
> -        )
> -
>          if not image_name and not task_id:
>              print("Specify either IMAGE-NAME or TASK-ID")
>              sys.exit(1)
> -
> +        try:

If we remove the ``try``/``except`` wouldn't it have the same effect?

::: taskcluster/taskgraph/docker.py:27
(Diff revision 2)
> +def load_image_by_name(image_name):
> +    context_path = os.path.join(GECKO, 'testing', 'docker', image_name)
> +    context_hash = docker.generate_context_hash(context_path)
> +
> +    image_index_url = INDEX_URL.format('mozilla-central', image_name, context_hash)
> +    print("Fetching {}".format(image_index_url))

print("Fetching", image_index_url)

::: taskcluster/taskgraph/docker.py:40
(Diff revision 2)
> +    # through), it is difficult to stream it.  So we download to disk and then
> +    # read it back.
> +    filename = 'temp-docker-image.tar'
> +
> +    artifact_url = ARTIFACT_URL.format(task_id, 'public/image.tar')
> +    print("Downloading {}".format(artifact_url))

print("Downloading", artifact_url)

::: taskcluster/taskgraph/docker.py:49
(Diff revision 2)
> +    tf = tarfile.open(filename)
> +    repositories = json.load(tf.extractfile('repositories'))
> +    name = repositories.keys()[0]
> +    tag = repositories[name].keys()[0]
> +    name = '{}:{}'.format(name, tag)
> +    print("Image name: {}".format(name))

print("Image name:", name)
Attachment #8760485 - Flags: review?(wcosta) → review+
Comment on attachment 8760486 [details]
Bug 1275409: remove mach_util;

https://reviewboard.mozilla.org/r/58038/#review55208

::: taskcluster/taskgraph/kind/legacy.py:64
(Diff revision 2)
> +    for dictionary in dicts:
> +        merged_dict.update(dictionary)
> +    return merged_dict
> +
> +def gaia_info():
> +    '''

nit: doc string should start at the same line as '''

::: taskcluster/taskgraph/kind/legacy.py:93
(Diff revision 2)
> +            'gaia_rev': gaia['git']['git_revision'],
> +            'gaia_ref': gaia['git']['branch'],
> +        }
> +
> +def configure_dependent_task(task_path, parameters, taskid, templates, build_treeherder_config):
> +    """

nit: doc string should start in the same line as """

::: taskcluster/taskgraph/kind/legacy.py:168
(Diff revision 2)
> +    except KeyError:
> +        pass
> +
> +def query_vcs_info(repository, revision):
> +    """Query the pushdate and pushid of a repository/revision.
> +    This is intended to be used on hg.mozilla.org/mozilla-central and

nit: I new line should be inserted here

::: taskcluster/taskgraph/kind/legacy.py:179
(Diff revision 2)
> +
> +    VCSInfo = namedtuple('VCSInfo', ['pushid', 'pushdate', 'changesets'])
> +
> +    try:
> +        import requests
> +        url = '%s/json-automationrelevance/%s' % (repository.rstrip('/'),

nit: good opportunity to replace these by `format`

::: taskcluster/taskgraph/kind/legacy.py:181
(Diff revision 2)
> +
> +    try:
> +        import requests
> +        url = '%s/json-automationrelevance/%s' % (repository.rstrip('/'),
> +                                                  revision)
> +        logger.debug("Querying version control for metadata: %s" % url)

`logger.debug` already supports format string

::: taskcluster/taskgraph/kind/legacy.py:194
(Diff revision 2)
> +        pushdate = contents['changesets'][-1]['pushdate'][0]
> +
> +        return VCSInfo(pushid, pushdate, changesets)
> +
> +    except Exception:
> +        logger.exception(

`exception` already supports format string
Attachment #8760486 - Flags: review?(wcosta) → review+
https://reviewboard.mozilla.org/r/58036/#review55194

> If we remove the ``try``/``except`` wouldn't it have the same effect?

No, the mach framework prints exceptions in a very verbose way with the string "this should not happen".  This try/except is the standard pattern.
https://reviewboard.mozilla.org/r/58038/#review55208

> nit: good opportunity to replace these by `format`

This is legacy code :)
Attachment #8760487 - Flags: review?(wcosta) → review+
Comment on attachment 8760487 [details]
Bug 1275409: move taskcluster_graph.commit_parser;

https://reviewboard.mozilla.org/r/58040/#review55338

::: taskcluster/taskgraph/util/legacy_commit_parser.py:24
(Diff revision 2)
>      'o': 'opt',
>      'd': 'debug'
>  }
>  
> +def parse_test_opts(input_str):
> +    '''

nit: doc string should start at the same line as '''

::: taskcluster/taskgraph/util/legacy_commit_parser.py:49
(Diff revision 2)
> +        cur_test['platforms'].insert(0, value.strip())
> +
> +    # This might be somewhat confusing but we parse the string _backwards_ so
> +    # there is no ambiguity over what state we are in.
> +    for char in reversed(input_str):
> +

nit: remove empty line

::: taskcluster/taskgraph/util/legacy_commit_parser.py:52
(Diff revision 2)
> +    # there is no ambiguity over what state we are in.
> +    for char in reversed(input_str):
> +
> +        # , indicates exiting a state
> +        if char == ',':
> +

nit: remove empty line

::: taskcluster/taskgraph/test/test_util_legacy_commit_parser.py:953
(Diff revision 2)
>          result, triggers = parse_commit(commit, jobs)
> -        self.assertEqual(expected, result)
> +        self.assertEqual(sorted(expected), sorted(result))
> +
> +
> +class TryTestParserTest(unittest.TestCase):
> +

nit: remove empty line
Comment on attachment 8760488 [details]
Bug 1275409: remove taskcluster_graph.build_task;

https://reviewboard.mozilla.org/r/58042/#review55340

::: taskcluster/taskgraph/kind/legacy.py:264
(Diff revision 2)
>  
> +class BuildTaskValidationException(Exception):
> +    pass
> +
> +def validate_build_task(task):
> +    '''

nit: docstring should be in the same line as '''

::: taskcluster/taskgraph/test/test_kind_legacy.py:29
(Diff revision 2)
>      def setUp(self):
>          self.kind = LegacyKind('/root', {})
>  
>  
> +class TestValidateBuildTask(unittest.TestCase):
> +

nit: remove empty line
Attachment #8760488 - Flags: review?(wcosta) → review+
Comment on attachment 8760489 [details]
Bug 1275409: remove unused aliases list in yaml files;

https://reviewboard.mozilla.org/r/58044/#review55342
Attachment #8760489 - Flags: review?(wcosta) → review+
Comment on attachment 8760490 [details]
Bug 1275409: remove remaining code in testing/taskcluster;

https://reviewboard.mozilla.org/r/58046/#review55346
Attachment #8760490 - Flags: review?(wcosta) → review+
Comment on attachment 8760491 [details]
Bug 1275409: remove unused legacy task definitions;

https://reviewboard.mozilla.org/r/58048/#review55348
Attachment #8760491 - Flags: review?(wcosta) → review+
Comment on attachment 8760492 [details]
Bug 1275409: move legacy task definitions into legacy kind;

https://reviewboard.mozilla.org/r/58050/#review55350
Attachment #8760492 - Flags: review?(wcosta) → review+
Attachment #8760493 - Flags: review?(wcosta) → review+
Comment on attachment 8760493 [details]
Bug 1275409: move legacy taskcluster-related scripts to taskcluster/scripts;

https://reviewboard.mozilla.org/r/58052/#review55352
Comment on attachment 8760480 [details]
Bug 1275409: remove testing/taskcluster/mach_commands.py, except 'mach taskcluster-load-image';

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58026/diff/1-2/
Comment on attachment 8760481 [details]
Bug 1275409: mock os.path.is{dir,file} too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58028/diff/2-3/
Comment on attachment 8760482 [details]
Bug 1275409: move templates to taskgraph.util;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58030/diff/2-3/
Comment on attachment 8760483 [details]
Bug 1275409: move taskcluster_graph.from_now to taskgraph.util.time;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58032/diff/2-3/
Comment on attachment 8760484 [details]
Bug 1275409: remove taskcluster_graph.transform;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58034/diff/2-3/
Comment on attachment 8760485 [details]
Bug 1275409: remove taskcluster_graph.image_builder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58036/diff/2-3/
Comment on attachment 8760486 [details]
Bug 1275409: remove mach_util;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58038/diff/2-3/
Comment on attachment 8760487 [details]
Bug 1275409: move taskcluster_graph.commit_parser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58040/diff/2-3/
Comment on attachment 8760488 [details]
Bug 1275409: remove taskcluster_graph.build_task;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58042/diff/2-3/
Comment on attachment 8760489 [details]
Bug 1275409: remove unused aliases list in yaml files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58044/diff/2-3/
Comment on attachment 8760490 [details]
Bug 1275409: remove remaining code in testing/taskcluster;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58046/diff/2-3/
Comment on attachment 8760491 [details]
Bug 1275409: remove unused legacy task definitions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58048/diff/2-3/
Comment on attachment 8760492 [details]
Bug 1275409: move legacy task definitions into legacy kind;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58050/diff/2-3/
Comment on attachment 8760493 [details]
Bug 1275409: move legacy taskcluster-related scripts to taskcluster/scripts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58052/diff/2-3/
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea44dbe1291
remove testing/taskcluster/mach_commands.py, except 'mach taskcluster-load-image'; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/c242defba0dd
mock os.path.is{dir,file} too; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c6c0ec3bc5
move templates to taskgraph.util; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f6b98f1ef6
move taskcluster_graph.from_now to taskgraph.util.time; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb228c042e37
remove taskcluster_graph.transform; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d7584daf9e
remove taskcluster_graph.image_builder; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ddcce18639b
remove mach_util; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/ade6f94708a1
move taskcluster_graph.commit_parser; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/4294729324ec
remove taskcluster_graph.build_task; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de0d9a60741
remove unused aliases list in yaml files; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a0072085180
remove remaining code in testing/taskcluster; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15619581cdc
remove unused legacy task definitions; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/c192b9b76fba
move legacy task definitions into legacy kind; r=wcosta
https://hg.mozilla.org/integration/mozilla-inbound/rev/d503738745c7
move legacy taskcluster-related scripts to taskcluster/scripts; r=wcosta
It turns out mozharness was looking for routes.json in the old location.
Attachment #8761233 - Flags: review?(mshal)
Comment on attachment 8761233 [details] [diff] [review]
routes-json.patch

I disagree with the notion that this should be "legacy", so we'll need to sort out what you intend this to look like in the future. And please test such things on try before landing them - there's no reason for this to be an emergency.
Attachment #8761233 - Flags: review?(mshal) → review+
please announce changes like this a bit more publicly as well as linking to the docs!
(In reply to Joel Maher (:jmaher) from comment #74)
> please announce changes like this a bit more publicly as well as linking to
> the docs!

We should talk in London about this. I'm not sure announcing it more would have alerted the right people.
when I couldn't find files, I looked for an annoucement on mozilla.tools.taskcluster and there was none, this is the second time in the last 2 weeks things have changed with no documentation in a bug or a newsgroup that existing configs will not work.

even the readthedocs page doesn't help as there is no mention that 'things were moved on June 8th in bug 1275409'.  Spending 30+ minutes trying to figure this out by big bugzilla queries, etc. just to find the moved file is a good sign that we should publicize this.

is there another channel that I should be looking at for news on major changes and how they affect existing patches?
I'm sorry you spent so much time tracing this down.  I noticed you suggested sending a note to a taskcluster list, and that certainly wouldn't have hurt (at least, it would have let you know.. I don't know how much overlap there is with everyone who might need to know).

This isn't a TC-specific issue, though.  I'm sure there are major refactors landing in the Gecko tree daily.  Is there documentation somewhere on how those should be announced?
there are large file moves in gecko, these take place about once/quarter.  What took me so long was 3 things:
1) the files moved so looking at the log didn't help as there was no file to find a history for
2) I found files put in a 'legacy' folder, this made me question if they were used or not
3) the bug related to the move to the legacy folder had no documentation to indicate we were using the files or not using them.

Some examples of posts to dev.platform for files being moved:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/GXZY0yrRHdw
https://groups.google.com/forum/#!searchin/mozilla.dev.platform/move$20files/mozilla.dev.platform/jsAZHKSjirM/o54RliUghEwJ

and gps posts about upcoming changes to build system related files:
https://groups.google.com/forum/#!searchin/mozilla.dev.platform/moving$20files/mozilla.dev.platform/hmUzuq7P31o/a3kQj4rZkAEJ

the good news is this is a problem because people are using taskcluster and have existing work that needs to be adapted to the new file structure :)
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.