-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add commits differentiation logic for incremental build #4207
Changes from all commits
53ec862
18e0761
5465959
3ac3caf
eaec41a
a93f1ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
--- | ||
schema-version: '1.0' | ||
schema-version: '1.1' | ||
build: | ||
name: OpenSearch Dashboards | ||
version: 2.12.0 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import logging | ||
import os | ||
from typing import List | ||
|
||
from manifests.build_manifest import BuildManifest | ||
from manifests.input_manifest import InputManifest | ||
|
||
|
||
class BuildIncremental: | ||
def __init__(self, input_manifest: InputManifest, distribution: str): | ||
self.distribution = distribution | ||
self.input_manifest = input_manifest | ||
|
||
# Given input manifest and return a list of what components changed and added. | ||
def commits_diff(self, input_manifest: InputManifest) -> List[str]: | ||
build_manifest_path = os.path.join(self.distribution, "builds", input_manifest.build.filename, "manifest.yml") | ||
if not os.path.exists(build_manifest_path): | ||
logging.info("Previous build manifest does not exist. Rebuilding Core.") | ||
return [input_manifest.build.name.replace(" ", "-")] | ||
previous_build_manifest = BuildManifest.from_path(build_manifest_path) | ||
stable_input_manifest = input_manifest.stable() | ||
if previous_build_manifest.build.version != stable_input_manifest.build.version: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this check is here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This came out of my mind when I was testing on local. When we are building artifacts in local, we don't have any sub-folder for different version. All built artifacts stored in the same directory following this pattern: So I believe at least we need to check we are incrementally building the same version artifacts. |
||
logging.info("The version of previous build manifest doesn't match the current input manifest. Rebuilding Core.") | ||
return [input_manifest.build.name.replace(" ", "-")] | ||
components = [] | ||
for component in stable_input_manifest.components.select(): | ||
if component.name not in previous_build_manifest.components: | ||
components.append(component.name) | ||
logging.info(f"Adding {component.name} since it is missing from previous build manifest") | ||
continue | ||
if component.ref != previous_build_manifest.components[component.name].commit_id: # type: ignore[attr-defined] | ||
components.append(component.name) | ||
logging.info(f"Adding {component.name} because it has different commit ID and needs to be rebuilt.") | ||
continue | ||
return components | ||
|
||
# Given updated plugins and look into the depends_on of all components to finalize a list of rebuilding components. | ||
def rebuild_plugins(self, changed_plugins: List, input_manifest: InputManifest) -> List[str]: | ||
if not changed_plugins: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be add this check in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes sure. This would be considered in the current implementation on the building process for incremental build. |
||
return [] | ||
|
||
if any(core in changed_plugins for core in ("OpenSearch", "OpenSearch-Dashboards")): | ||
logging.info("Core engine has new changes, rebuilding all components.") | ||
return [component.name for component in input_manifest.components.select()] | ||
|
||
queue = changed_plugins | ||
zelinh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rebuild_list = [] | ||
|
||
rebuild_list.append("OpenSearch-Dashboards") if input_manifest.build.filename == "opensearch-dashboards" else rebuild_list | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did not understand this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or for OSD plugins to rebuild, we need OSD to be rebuilt again? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. OSD doesn't have any those built-tool available just like OS. Every time we rebuild any of dashboards plugin, we have to build OSD core. |
||
while queue: | ||
plugin = queue.pop(0) | ||
rebuild_list.append(plugin) if plugin not in rebuild_list else rebuild_list | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to make sure there is no duplicate in the |
||
for dependent_plugin in input_manifest.plugins_depend_on(plugin): | ||
queue.append(dependent_plugin) if dependent_plugin not in rebuild_list else queue | ||
|
||
logging.info(f"Rebuilding list is {rebuild_list}") | ||
return rebuild_list |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
import sys | ||
|
||
from build_workflow.build_args import BuildArgs | ||
from build_workflow.build_incremental import BuildIncremental | ||
from build_workflow.build_recorder import BuildRecorder | ||
from build_workflow.build_target import BuildTarget | ||
from build_workflow.builders import Builders | ||
|
@@ -41,6 +42,12 @@ | |
|
||
output_dir = BuildOutputDir(manifest.build.filename, args.distribution).dir | ||
|
||
if args.incremental: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a placeholder for the next PR and it's just logging a list of plugins for rebuilding. I have complete tests for all those functions within There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It still is part of the code flow and should be tested, if not in this PR then in subsequent PRs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will absolutely add tests in my following PR. |
||
buildIncremental = BuildIncremental(manifest, args.distribution) | ||
list_of_updated_plugins = buildIncremental.commits_diff(manifest) | ||
logging.info(f"Plugins for incremental build: {buildIncremental.rebuild_plugins(list_of_updated_plugins, manifest)}") | ||
return 0 | ||
Comment on lines
+45
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pointing the exact question from the overview here: Wondering what difference is this making in overall build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah as I stated in the description of this issue, these additions in |
||
|
||
with TemporaryDirectory(keep=args.keep, chdir=True) as work_dir: | ||
logging.info(f"Building in {work_dir.name}") | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming we will be downloading the build-manifest and storing it in the path mentioned before this logic executes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the scope of the workflow on python side, we would assume we already have the
builds
folder along with build-manifest in local directory. We would need to download this entirebuilds
folder on the CI level when we are running on Jenkins.