Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Bug 1625126 - Cache external dependencies in a single task and let gr… #10316

Merged
merged 1 commit into from
May 13, 2020

Conversation

JohanLorenzo
Copy link
Contributor

…adle tasks use it

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@JohanLorenzo JohanLorenzo force-pushed the bug-1625126 branch 2 times, most recently from e95d2db to 088022f Compare May 4, 2020 14:19
@codecov-io
Copy link

codecov-io commented May 6, 2020

Codecov Report

Merging #10316 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #10316   +/-   ##
=========================================
  Coverage     19.53%   19.53%           
  Complexity      601      601           
=========================================
  Files           357      357           
  Lines         14421    14421           
  Branches       1940     1940           
=========================================
  Hits           2817     2817           
  Misses        11348    11348           
  Partials        256      256           

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 824dedb...244fd7d. Read the comment docs.

Copy link
Contributor Author

@JohanLorenzo JohanLorenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is finally ready for review. I found what was the root cause of the dependencies task timing out: #10439. Once that problem was worked around, it was fairly easy to get that task run under 30 minutes 👌

The rest of the PR is similar to the one we had in A-C.

I tested :

provisioner: 'mobile-{level}'
implementation: docker-worker
os: linux
worker-type: b-linux-large
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This instance type is used by android-components. Using it reduces the time taken by the dependencies task from 30-40 minutes down to 20-30.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to start looking into reducing the number of build variants. I believe that this will also significantly reduce the assemble time.



# ./gradlew lint is missing because of https://github.com/mozilla-mobile/fenix/issues/10439. So far,
# we're lucky and the dependencies it fetches are found elsewhere.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the regular builds, I checked the nightly, beta, and production graphs and none has complained about the lack of ./gradlew lint 👍

Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

# %ARG DOCKER_IMAGE_PARENT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is this comment a leftover from testing? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! This line is actually a template shortcut used by taskgraph. It allows us to reference a parent docker image which was built in Taskcluster too.

provisioner: 'mobile-{level}'
implementation: docker-worker
os: linux
worker-type: b-linux-large
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to start looking into reducing the number of build variants. I believe that this will also significantly reduce the assemble time.

@JohanLorenzo JohanLorenzo merged commit 714c2c8 into mozilla-mobile:master May 13, 2020
@JohanLorenzo JohanLorenzo deleted the bug-1625126 branch May 13, 2020 16:37
@liuche liuche mentioned this pull request May 19, 2020
32 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants