* [PATCH v3 1/3] buildman: Support building within a Python venv
2024-08-15 19:57 [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in Simon Glass
@ 2024-08-15 19:57 ` Simon Glass
2024-09-06 0:53 ` Simon Glass
2024-08-15 19:57 ` [PATCH v3 2/3] buildman: Allow skipping the dtc build Simon Glass
` (2 subsequent siblings)
3 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-08-15 19:57 UTC (permalink / raw)
To: U-Boot Mailing List
Cc: Tom Rini, Simon Glass, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt, Quentin Schulz
The Python virtualenv tool sets up a few things in the environment,
putting its path first in the PATH environment variable and setting up
a sys.prefix different from the sys.base_prefix value.
At present buildman puts the toolchain path first in PATH so that it can
be found easily during the build. For sandbox this causes problems since
/usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
PATH variable. As a result, the venv is partially disabled.
The result is that sandbox builds within a venv ignore the venv, e.g.
when looking for packages.
Correct this by detecting the venv and adding the toolchain path after
the venv path.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
(no changes since v2)
Changes in v2:
- Fix 'envronment' typo
tools/buildman/bsettings.py | 3 ++
tools/buildman/test.py | 84 +++++++++++++++++++++++++++++++++++++
tools/buildman/toolchain.py | 31 ++++++++++++--
3 files changed, 114 insertions(+), 4 deletions(-)
diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
index aea724fc559..a7358cfc08a 100644
--- a/tools/buildman/bsettings.py
+++ b/tools/buildman/bsettings.py
@@ -31,6 +31,9 @@ def setup(fname=''):
def add_file(data):
settings.read_file(io.StringIO(data))
+def add_section(name):
+ settings.add_section(name)
+
def get_items(section):
"""Get the items from a section of the config.
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index bfad3093030..6191af95445 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -148,6 +148,7 @@ class TestBuild(unittest.TestCase):
self.toolchains.Add('arm-linux-gcc', test=False)
self.toolchains.Add('sparc-linux-gcc', test=False)
self.toolchains.Add('powerpc-linux-gcc', test=False)
+ self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
self.toolchains.Add('gcc', test=False)
# Avoid sending any output
@@ -868,6 +869,89 @@ class TestBuild(unittest.TestCase):
self.assertEqual([4, 5], control.read_procs(tmpdir))
self.assertEqual(self.finish_time, self.cur_time)
+ def call_make_environment(self, tchn, full_path, in_env=None):
+ """Call Toolchain.MakeEnvironment() and process the result
+
+ Args:
+ tchn (Toolchain): Toolchain to use
+ full_path (bool): True to return the full path in CROSS_COMPILE
+ rather than adding it to the PATH variable
+ in_env (dict): Input environment to use, None to use current env
+
+ Returns:
+ tuple:
+ dict: Changes that MakeEnvironment has made to the environment
+ key: Environment variable that was changed
+ value: New value (for PATH this only includes components
+ which were added)
+ str: Full value of the new PATH variable
+ """
+ env = tchn.MakeEnvironment(full_path, env=in_env)
+
+ # Get the original environment
+ orig_env = dict(os.environb if in_env is None else in_env)
+ orig_path = orig_env[b'PATH'].split(b':')
+
+ # Find new variables
+ diff = dict((k, env[k]) for k in env if orig_env.get(k) != env[k])
+
+ # Find new / different path components
+ diff_path = None
+ new_path = None
+ if b'PATH' in diff:
+ new_path = diff[b'PATH'].split(b':')
+ diff_paths = [p for p in new_path if p not in orig_path]
+ diff_path = b':'.join(p for p in new_path if p not in orig_path)
+ if diff_path:
+ diff[b'PATH'] = diff_path
+ else:
+ del diff[b'PATH']
+ return diff, new_path
+
+ def test_toolchain_env(self):
+ """Test PATH and other environment settings for toolchains"""
+ # Use a toolchain which has a path, so that full_path makes a difference
+ tchn = self.toolchains.Select('aarch64')
+
+ # Normal cases
+ diff = self.call_make_environment(tchn, full_path=False)[0]
+ self.assertEqual(
+ {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
+ b'PATH': b'/path/to'}, diff)
+
+ diff = self.call_make_environment(tchn, full_path=True)[0]
+ self.assertEqual(
+ {b'CROSS_COMPILE': b'/path/to/aarch64-linux-', b'LC_ALL': b'C'},
+ diff)
+
+ # When overriding the toolchain, only LC_ALL should be set
+ tchn.override_toolchain = True
+ diff = self.call_make_environment(tchn, full_path=True)[0]
+ self.assertEqual({b'LC_ALL': b'C'}, diff)
+
+ # Test that virtualenv is handled correctly
+ tchn.override_toolchain = False
+ sys.prefix = '/some/venv'
+ env = dict(os.environb)
+ env[b'PATH'] = b'/some/venv/bin:other/things'
+ tchn.path = '/my/path'
+ diff, diff_path = self.call_make_environment(tchn, False, env)
+
+ self.assertIn(b'PATH', diff)
+ self.assertEqual([b'/some/venv/bin', b'/my/path', b'other/things'],
+ diff_path)
+ self.assertEqual(
+ {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
+ b'PATH': b'/my/path'}, diff)
+
+ # Handle a toolchain wrapper
+ tchn.path = ''
+ bsettings.add_section('toolchain-wrapper')
+ bsettings.set_item('toolchain-wrapper', 'my-wrapper', 'fred')
+ diff = self.call_make_environment(tchn, full_path=True)[0]
+ self.assertEqual(
+ {b'CROSS_COMPILE': b'fred aarch64-linux-', b'LC_ALL': b'C'}, diff)
+
if __name__ == "__main__":
unittest.main()
diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
index 324ad0e0821..6ca79c2c0f9 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -172,13 +172,14 @@ class Toolchain:
else:
raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
- def MakeEnvironment(self, full_path):
+ def MakeEnvironment(self, full_path, env=None):
"""Returns an environment for using the toolchain.
This takes the current environment and adds CROSS_COMPILE so that
the tool chain will operate correctly. This also disables localized
output and possibly Unicode encoded output of all build tools by
- adding LC_ALL=C.
+ adding LC_ALL=C. For the case where full_path is False, it prepends
+ the toolchain to PATH
Note that os.environb is used to obtain the environment, since in some
cases the environment many contain non-ASCII characters and we see
@@ -187,15 +188,21 @@ class Toolchain:
UnicodeEncodeError: 'utf-8' codec can't encode characters in position
569-570: surrogates not allowed
+ When running inside a Python venv, care is taken not to put the
+ toolchain path before the venv path, so that builds initiated by
+ buildman will still respect the venv.
+
Args:
full_path: Return the full path in CROSS_COMPILE and don't set
PATH
+ env (dict of bytes): Original environment, used for testing
Returns:
Dict containing the (bytes) environment to use. This is based on the
current environment, with changes as needed to CROSS_COMPILE, PATH
and LC_ALL.
"""
- env = dict(os.environb)
+ env = dict(env or os.environb)
+
wrapper = self.GetWrapper()
if self.override_toolchain:
@@ -206,7 +213,23 @@ class Toolchain:
wrapper + os.path.join(self.path, self.cross))
else:
env[b'CROSS_COMPILE'] = tools.to_bytes(wrapper + self.cross)
- env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
+
+ # Detect a Python virtualenv and avoid defeating it
+ if sys.prefix != sys.base_prefix:
+ paths = env[b'PATH'].split(b':')
+ new_paths = []
+ to_insert = tools.to_bytes(self.path)
+ insert_after = tools.to_bytes(sys.prefix)
+ for path in paths:
+ new_paths.append(path)
+ if to_insert and path.startswith(insert_after):
+ new_paths.append(to_insert)
+ to_insert = None
+ if to_insert:
+ new_paths.append(to_insert)
+ env[b'PATH'] = b':'.join(new_paths)
+ else:
+ env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
env[b'LC_ALL'] = b'C'
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v3 1/3] buildman: Support building within a Python venv
2024-08-15 19:57 ` [PATCH v3 1/3] buildman: Support building within a Python venv Simon Glass
@ 2024-09-06 0:53 ` Simon Glass
0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2024-09-06 0:53 UTC (permalink / raw)
To: U-Boot Mailing List
Cc: Tom Rini, Andrejs Cainikovs, Brandon Maier, Heinrich Schuchardt,
Quentin Schulz
Hi Tom,
On Thu, 15 Aug 2024 at 13:57, Simon Glass <sjg@chromium.org> wrote:
>
> The Python virtualenv tool sets up a few things in the environment,
> putting its path first in the PATH environment variable and setting up
> a sys.prefix different from the sys.base_prefix value.
>
> At present buildman puts the toolchain path first in PATH so that it can
> be found easily during the build. For sandbox this causes problems since
> /usr/bin/gcc (for example) results in '/usr/bin' being prepended to the
> PATH variable. As a result, the venv is partially disabled.
>
> The result is that sandbox builds within a venv ignore the venv, e.g.
> when looking for packages.
>
> Correct this by detecting the venv and adding the toolchain path after
> the venv path.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Fix 'envronment' typo
>
> tools/buildman/bsettings.py | 3 ++
> tools/buildman/test.py | 84 +++++++++++++++++++++++++++++++++++++
> tools/buildman/toolchain.py | 31 ++++++++++++--
> 3 files changed, 114 insertions(+), 4 deletions(-)
>
> diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
> index aea724fc559..a7358cfc08a 100644
> --- a/tools/buildman/bsettings.py
> +++ b/tools/buildman/bsettings.py
> @@ -31,6 +31,9 @@ def setup(fname=''):
> def add_file(data):
> settings.read_file(io.StringIO(data))
>
> +def add_section(name):
> + settings.add_section(name)
> +
> def get_items(section):
> """Get the items from a section of the config.
>
> diff --git a/tools/buildman/test.py b/tools/buildman/test.py
> index bfad3093030..6191af95445 100644
> --- a/tools/buildman/test.py
> +++ b/tools/buildman/test.py
> @@ -148,6 +148,7 @@ class TestBuild(unittest.TestCase):
> self.toolchains.Add('arm-linux-gcc', test=False)
> self.toolchains.Add('sparc-linux-gcc', test=False)
> self.toolchains.Add('powerpc-linux-gcc', test=False)
> + self.toolchains.Add('/path/to/aarch64-linux-gcc', test=False)
> self.toolchains.Add('gcc', test=False)
>
> # Avoid sending any output
> @@ -868,6 +869,89 @@ class TestBuild(unittest.TestCase):
> self.assertEqual([4, 5], control.read_procs(tmpdir))
> self.assertEqual(self.finish_time, self.cur_time)
>
> + def call_make_environment(self, tchn, full_path, in_env=None):
> + """Call Toolchain.MakeEnvironment() and process the result
> +
> + Args:
> + tchn (Toolchain): Toolchain to use
> + full_path (bool): True to return the full path in CROSS_COMPILE
> + rather than adding it to the PATH variable
> + in_env (dict): Input environment to use, None to use current env
> +
> + Returns:
> + tuple:
> + dict: Changes that MakeEnvironment has made to the environment
> + key: Environment variable that was changed
> + value: New value (for PATH this only includes components
> + which were added)
> + str: Full value of the new PATH variable
> + """
> + env = tchn.MakeEnvironment(full_path, env=in_env)
> +
> + # Get the original environment
> + orig_env = dict(os.environb if in_env is None else in_env)
> + orig_path = orig_env[b'PATH'].split(b':')
> +
> + # Find new variables
> + diff = dict((k, env[k]) for k in env if orig_env.get(k) != env[k])
> +
> + # Find new / different path components
> + diff_path = None
> + new_path = None
> + if b'PATH' in diff:
> + new_path = diff[b'PATH'].split(b':')
> + diff_paths = [p for p in new_path if p not in orig_path]
> + diff_path = b':'.join(p for p in new_path if p not in orig_path)
> + if diff_path:
> + diff[b'PATH'] = diff_path
> + else:
> + del diff[b'PATH']
> + return diff, new_path
> +
> + def test_toolchain_env(self):
> + """Test PATH and other environment settings for toolchains"""
> + # Use a toolchain which has a path, so that full_path makes a difference
> + tchn = self.toolchains.Select('aarch64')
> +
> + # Normal cases
> + diff = self.call_make_environment(tchn, full_path=False)[0]
> + self.assertEqual(
> + {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
> + b'PATH': b'/path/to'}, diff)
> +
> + diff = self.call_make_environment(tchn, full_path=True)[0]
> + self.assertEqual(
> + {b'CROSS_COMPILE': b'/path/to/aarch64-linux-', b'LC_ALL': b'C'},
> + diff)
> +
> + # When overriding the toolchain, only LC_ALL should be set
> + tchn.override_toolchain = True
> + diff = self.call_make_environment(tchn, full_path=True)[0]
> + self.assertEqual({b'LC_ALL': b'C'}, diff)
> +
> + # Test that virtualenv is handled correctly
> + tchn.override_toolchain = False
> + sys.prefix = '/some/venv'
> + env = dict(os.environb)
> + env[b'PATH'] = b'/some/venv/bin:other/things'
> + tchn.path = '/my/path'
> + diff, diff_path = self.call_make_environment(tchn, False, env)
> +
> + self.assertIn(b'PATH', diff)
> + self.assertEqual([b'/some/venv/bin', b'/my/path', b'other/things'],
> + diff_path)
> + self.assertEqual(
> + {b'CROSS_COMPILE': b'aarch64-linux-', b'LC_ALL': b'C',
> + b'PATH': b'/my/path'}, diff)
> +
> + # Handle a toolchain wrapper
> + tchn.path = ''
> + bsettings.add_section('toolchain-wrapper')
> + bsettings.set_item('toolchain-wrapper', 'my-wrapper', 'fred')
> + diff = self.call_make_environment(tchn, full_path=True)[0]
> + self.assertEqual(
> + {b'CROSS_COMPILE': b'fred aarch64-linux-', b'LC_ALL': b'C'}, diff)
> +
>
> if __name__ == "__main__":
> unittest.main()
> diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
> index 324ad0e0821..6ca79c2c0f9 100644
> --- a/tools/buildman/toolchain.py
> +++ b/tools/buildman/toolchain.py
> @@ -172,13 +172,14 @@ class Toolchain:
> else:
> raise ValueError('Unknown arg to GetEnvArgs (%d)' % which)
>
> - def MakeEnvironment(self, full_path):
> + def MakeEnvironment(self, full_path, env=None):
> """Returns an environment for using the toolchain.
>
> This takes the current environment and adds CROSS_COMPILE so that
> the tool chain will operate correctly. This also disables localized
> output and possibly Unicode encoded output of all build tools by
> - adding LC_ALL=C.
> + adding LC_ALL=C. For the case where full_path is False, it prepends
> + the toolchain to PATH
>
> Note that os.environb is used to obtain the environment, since in some
> cases the environment many contain non-ASCII characters and we see
> @@ -187,15 +188,21 @@ class Toolchain:
> UnicodeEncodeError: 'utf-8' codec can't encode characters in position
> 569-570: surrogates not allowed
>
> + When running inside a Python venv, care is taken not to put the
> + toolchain path before the venv path, so that builds initiated by
> + buildman will still respect the venv.
> +
> Args:
> full_path: Return the full path in CROSS_COMPILE and don't set
> PATH
> + env (dict of bytes): Original environment, used for testing
> Returns:
> Dict containing the (bytes) environment to use. This is based on the
> current environment, with changes as needed to CROSS_COMPILE, PATH
> and LC_ALL.
> """
> - env = dict(os.environb)
> + env = dict(env or os.environb)
> +
> wrapper = self.GetWrapper()
>
> if self.override_toolchain:
> @@ -206,7 +213,23 @@ class Toolchain:
> wrapper + os.path.join(self.path, self.cross))
> else:
> env[b'CROSS_COMPILE'] = tools.to_bytes(wrapper + self.cross)
> - env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
> +
> + # Detect a Python virtualenv and avoid defeating it
> + if sys.prefix != sys.base_prefix:
> + paths = env[b'PATH'].split(b':')
> + new_paths = []
> + to_insert = tools.to_bytes(self.path)
> + insert_after = tools.to_bytes(sys.prefix)
> + for path in paths:
> + new_paths.append(path)
> + if to_insert and path.startswith(insert_after):
> + new_paths.append(to_insert)
> + to_insert = None
> + if to_insert:
> + new_paths.append(to_insert)
> + env[b'PATH'] = b':'.join(new_paths)
> + else:
> + env[b'PATH'] = tools.to_bytes(self.path) + b':' + env[b'PATH']
>
> env[b'LC_ALL'] = b'C'
>
> --
> 2.34.1
>
Can this one be picked up please? It fixes something and Jerome's
series should really go on top of it.
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-15 19:57 [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in Simon Glass
2024-08-15 19:57 ` [PATCH v3 1/3] buildman: Support building within a Python venv Simon Glass
@ 2024-08-15 19:57 ` Simon Glass
2024-08-16 17:22 ` Tom Rini
2024-08-15 19:57 ` [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds Simon Glass
2024-09-06 22:37 ` (subset) [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in Tom Rini
3 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-08-15 19:57 UTC (permalink / raw)
To: U-Boot Mailing List
Cc: Tom Rini, Simon Glass, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
For most boards, the device-tree compiler is built in-tree, ignoring the
system version. Add a special option to skip this build. This can be
useful when the system dtc is up-to-date, as it speeds up the build.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)
tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
tools/buildman/builderthread.py | 4 ++--
tools/buildman/buildman.rst | 3 +++
tools/buildman/cmdline.py | 2 ++
tools/buildman/control.py | 3 ++-
tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
6 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index c4384f53e8d..4090d328b30 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -22,6 +22,7 @@ from buildman import toolchain
from patman import gitutil
from u_boot_pylib import command
from u_boot_pylib import terminal
+from u_boot_pylib import tools
from u_boot_pylib.terminal import tprint
# This indicates an new int or hex Kconfig property with no default
@@ -263,7 +264,8 @@ class Builder:
adjust_cfg=None, allow_missing=False, no_lto=False,
reproducible_builds=False, force_build=False,
force_build_failures=False, force_reconfig=False,
- in_tree=False, force_config_on_failure=False, make_func=None):
+ in_tree=False, force_config_on_failure=False, make_func=None,
+ dtc_skip=False):
"""Create a new Builder object
Args:
@@ -312,6 +314,7 @@ class Builder:
force_config_on_failure (bool): Reconfigure the build before
retrying a failed build
make_func (function): Function to call to run 'make'
+ dtc_skip (bool): True to skip building dtc and use the system one
"""
self.toolchains = toolchains
self.base_dir = base_dir
@@ -354,6 +357,12 @@ class Builder:
self.in_tree = in_tree
self.force_config_on_failure = force_config_on_failure
self.fallback_mrproper = fallback_mrproper
+ if dtc_skip:
+ self.dtc = shutil.which('dtc')
+ if not self.dtc:
+ raise ValueError('Cannot find dtc')
+ else:
+ self.dtc = None
if not self.squash_config_y:
self.config_filenames += EXTRA_CONFIG_FILENAMES
@@ -407,6 +416,22 @@ class Builder:
def signal_handler(self, signal, frame):
sys.exit(1)
+ def make_environment(self, toolchain):
+ """Create the environment to use for building
+
+ Args:
+ toolchain (Toolchain): Toolchain to use for building
+
+ Returns:
+ dict:
+ key (str): Variable name
+ value (str): Variable value
+ """
+ env = toolchain.MakeEnvironment(self.full_path)
+ if self.dtc:
+ env[b'DTC'] = tools.to_bytes(self.dtc)
+ return env
+
def set_display_options(self, show_errors=False, show_sizes=False,
show_detail=False, show_bloat=False,
list_error_boards=False, show_config=False,
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index bbe2f6f0d24..54f3c02c844 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -406,7 +406,7 @@ class BuilderThread(threading.Thread):
the next incremental build
"""
# Set up the environment and command line
- env = self.toolchain.MakeEnvironment(self.builder.full_path)
+ env = self.builder.make_environment(self.toolchain)
mkdir(out_dir)
args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
@@ -574,7 +574,7 @@ class BuilderThread(threading.Thread):
outf.write(f'{result.return_code}')
# Write out the image and function size information and an objdump
- env = result.toolchain.MakeEnvironment(self.builder.full_path)
+ env = self.builder.make_environment(self.toolchain)
with open(os.path.join(build_dir, 'out-env'), 'wb') as outf:
for var in sorted(env.keys()):
outf.write(b'%s="%s"' % (var, env[var]))
diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index b8ff3bf1ab2..e873611e596 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -1030,6 +1030,9 @@ of the source tree, thus allowing rapid tested evolution of the code::
./tools/buildman/buildman -Pr tegra
+Note also the `--dtc-skip` option which uses the system device-tree compiler to
+avoid needing to build it for each board. This can save 10-20% of build time.
+An alternative is to set DTC=/path/to/dtc when running buildman.
Checking configuration
----------------------
diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index 544a391a464..7573e5bdfe8 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -46,6 +46,8 @@ def add_upto_m(parser):
help='Show detailed size delta for each board in the -S summary')
parser.add_argument('-D', '--debug', action='store_true',
help='Enabling debugging (provides a full traceback on error)')
+ parser.add_argument('--dtc-skip', action='store_true', default=False,
+ help='Skip building of dtc and use the system version')
parser.add_argument('-e', '--show_errors', action='store_true',
default=False, help='Show errors and warnings')
parser.add_argument('-E', '--warnings-as-errors', action='store_true',
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index d3d027f02ab..55d4d770c5c 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -809,7 +809,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
force_build = args.force_build,
force_build_failures = args.force_build_failures,
force_reconfig = args.force_reconfig, in_tree = args.in_tree,
- force_config_on_failure=not args.quick, make_func=make_func)
+ force_config_on_failure=not args.quick, make_func=make_func,
+ dtc_skip=args.dtc_skip)
TEST_BUILDER = builder
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index 6191af95445..91f7a3a475a 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -952,6 +952,37 @@ class TestBuild(unittest.TestCase):
self.assertEqual(
{b'CROSS_COMPILE': b'fred aarch64-linux-', b'LC_ALL': b'C'}, diff)
+ def test_skip_dtc(self):
+ """Test skipping building the dtc tool"""
+ old_path = os.getenv('PATH')
+ try:
+ os.environ['PATH'] = self.base_dir
+
+ # Check a missing tool
+ with self.assertRaises(ValueError) as exc:
+ builder.Builder(self.toolchains, self.base_dir, None, 0, 2,
+ dtc_skip=True)
+ self.assertIn('Cannot find dtc', str(exc.exception))
+
+ # Create a fake tool to use
+ dtc = os.path.join(self.base_dir, 'dtc')
+ tools.write_file(dtc, b'xx')
+ os.chmod(dtc, 0o777)
+
+ build = builder.Builder(self.toolchains, self.base_dir, None, 0, 2,
+ dtc_skip=True)
+ toolchain = self.toolchains.Select('arm')
+ env = build.make_environment(toolchain)
+ self.assertIn(b'DTC', env)
+
+ # Try the normal case, i.e. not skipping the dtc build
+ build = builder.Builder(self.toolchains, self.base_dir, None, 0, 2)
+ toolchain = self.toolchains.Select('arm')
+ env = build.make_environment(toolchain)
+ self.assertNotIn(b'DTC', env)
+ finally:
+ os.environ['PATH'] = old_path
+
if __name__ == "__main__":
unittest.main()
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-15 19:57 ` [PATCH v3 2/3] buildman: Allow skipping the dtc build Simon Glass
@ 2024-08-16 17:22 ` Tom Rini
2024-08-16 23:53 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-08-16 17:22 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> For most boards, the device-tree compiler is built in-tree, ignoring the
> system version. Add a special option to skip this build. This can be
> useful when the system dtc is up-to-date, as it speeds up the build.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
> tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> tools/buildman/builderthread.py | 4 ++--
> tools/buildman/buildman.rst | 3 +++
> tools/buildman/cmdline.py | 2 ++
> tools/buildman/control.py | 3 ++-
> tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> 6 files changed, 66 insertions(+), 4 deletions(-)
We should probably do this more generically, outside of buildman. We
have scripts/dtc-version.sh and if the system version isn't new enough
(and we just need to define whatever the minimum version is), then we
build our (not currently that new anymore) dtc instead.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-16 17:22 ` Tom Rini
@ 2024-08-16 23:53 ` Simon Glass
2024-08-22 3:00 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-08-16 23:53 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
>
> > For most boards, the device-tree compiler is built in-tree, ignoring the
> > system version. Add a special option to skip this build. This can be
> > useful when the system dtc is up-to-date, as it speeds up the build.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > tools/buildman/builderthread.py | 4 ++--
> > tools/buildman/buildman.rst | 3 +++
> > tools/buildman/cmdline.py | 2 ++
> > tools/buildman/control.py | 3 ++-
> > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > 6 files changed, 66 insertions(+), 4 deletions(-)
>
> We should probably do this more generically, outside of buildman. We
> have scripts/dtc-version.sh and if the system version isn't new enough
> (and we just need to define whatever the minimum version is), then we
> build our (not currently that new anymore) dtc instead.
Yes I think I did a patch for that ages ago [1], but it was rejected.
I'd be very happy for it to be applied as I think it is a better
solution than this one.
I see that some poor sod tried to do this in Linux this morning.
Regards,
Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20200427002929.239379-2-sjg@chromium.org/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-16 23:53 ` Simon Glass
@ 2024-08-22 3:00 ` Simon Glass
2024-08-22 14:10 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-08-22 3:00 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> >
> > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > system version. Add a special option to skip this build. This can be
> > > useful when the system dtc is up-to-date, as it speeds up the build.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > tools/buildman/builderthread.py | 4 ++--
> > > tools/buildman/buildman.rst | 3 +++
> > > tools/buildman/cmdline.py | 2 ++
> > > tools/buildman/control.py | 3 ++-
> > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > 6 files changed, 66 insertions(+), 4 deletions(-)
> >
> > We should probably do this more generically, outside of buildman. We
> > have scripts/dtc-version.sh and if the system version isn't new enough
> > (and we just need to define whatever the minimum version is), then we
> > build our (not currently that new anymore) dtc instead.
>
> Yes I think I did a patch for that ages ago [1], but it was rejected.
>
> I'd be very happy for it to be applied as I think it is a better
> solution than this one.
>
> I see that some poor sod tried to do this in Linux this morning.
Any thoughts on that patch?
Also I do see one problem. Newer dtc version produce a lot of
warnings, which causes CI to fail. So if we always use the newest
version, people are going to see a ton of warnings when they run
locally. Am I missing something here?
Regards,
SImon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-22 3:00 ` Simon Glass
@ 2024-08-22 14:10 ` Tom Rini
2024-08-22 14:15 ` Simon Glass
2024-09-01 20:09 ` Simon Glass
0 siblings, 2 replies; 27+ messages in thread
From: Tom Rini @ 2024-08-22 14:10 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]
On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tom,
> >
> > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > >
> > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > system version. Add a special option to skip this build. This can be
> > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > tools/buildman/builderthread.py | 4 ++--
> > > > tools/buildman/buildman.rst | 3 +++
> > > > tools/buildman/cmdline.py | 2 ++
> > > > tools/buildman/control.py | 3 ++-
> > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > >
> > > We should probably do this more generically, outside of buildman. We
> > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > (and we just need to define whatever the minimum version is), then we
> > > build our (not currently that new anymore) dtc instead.
> >
> > Yes I think I did a patch for that ages ago [1], but it was rejected.
> >
> > I'd be very happy for it to be applied as I think it is a better
> > solution than this one.
> >
> > I see that some poor sod tried to do this in Linux this morning.
>
> Any thoughts on that patch?
I'm open to re-considering [1] again, but we need to handle the warning
problem first. That means...
> Also I do see one problem. Newer dtc version produce a lot of
> warnings, which causes CI to fail. So if we always use the newest
> version, people are going to see a ton of warnings when they run
> locally. Am I missing something here?
Well, it would be great to get our Kbuild logic anywhere close to
in-sync again with upstream. But syncing up the disabling warning flags
shouldn't be too hard.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-22 14:10 ` Tom Rini
@ 2024-08-22 14:15 ` Simon Glass
2024-09-01 20:09 ` Simon Glass
1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2024-08-22 14:15 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > >
> > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > system version. Add a special option to skip this build. This can be
> > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > tools/buildman/buildman.rst | 3 +++
> > > > > tools/buildman/cmdline.py | 2 ++
> > > > > tools/buildman/control.py | 3 ++-
> > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > >
> > > > We should probably do this more generically, outside of buildman. We
> > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > (and we just need to define whatever the minimum version is), then we
> > > > build our (not currently that new anymore) dtc instead.
> > >
> > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > >
> > > I'd be very happy for it to be applied as I think it is a better
> > > solution than this one.
> > >
> > > I see that some poor sod tried to do this in Linux this morning.
> >
> > Any thoughts on that patch?
>
> I'm open to re-considering [1] again, but we need to handle the warning
> problem first. That means...
>
> > Also I do see one problem. Newer dtc version produce a lot of
> > warnings, which causes CI to fail. So if we always use the newest
> > version, people are going to see a ton of warnings when they run
> > locally. Am I missing something here?
>
> Well, it would be great to get our Kbuild logic anywhere close to
> in-sync again with upstream. But syncing up the disabling warning flags
> shouldn't be too hard.
Have you talked to Linaro about taking this on? It becomes more and
more important as time goes on.
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-08-22 14:10 ` Tom Rini
2024-08-22 14:15 ` Simon Glass
@ 2024-09-01 20:09 ` Simon Glass
2024-09-02 15:39 ` Tom Rini
1 sibling, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-09-01 20:09 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > >
> > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > system version. Add a special option to skip this build. This can be
> > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > tools/buildman/buildman.rst | 3 +++
> > > > > tools/buildman/cmdline.py | 2 ++
> > > > > tools/buildman/control.py | 3 ++-
> > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > >
> > > > We should probably do this more generically, outside of buildman. We
> > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > (and we just need to define whatever the minimum version is), then we
> > > > build our (not currently that new anymore) dtc instead.
> > >
> > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > >
> > > I'd be very happy for it to be applied as I think it is a better
> > > solution than this one.
> > >
> > > I see that some poor sod tried to do this in Linux this morning.
> >
> > Any thoughts on that patch?
>
> I'm open to re-considering [1] again, but we need to handle the warning
> problem first. That means...
>
> > Also I do see one problem. Newer dtc version produce a lot of
> > warnings, which causes CI to fail. So if we always use the newest
> > version, people are going to see a ton of warnings when they run
> > locally. Am I missing something here?
>
> Well, it would be great to get our Kbuild logic anywhere close to
> in-sync again with upstream. But syncing up the disabling warning flags
> shouldn't be too hard.
So, coming back to this patch, the nice thing about it is that it is
deterministic. So people who build U-Boot and don't want funky
behaviour will be happy. It will use the internal dtc by default. To
use the external one, you must provide an option.
This patch only affects buildman, but as you can see the mechanism it
uses is to set the DTC variable, which people can do without buildman.
It's just a convenience, but useful enough to have a flag, I believe.
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-01 20:09 ` Simon Glass
@ 2024-09-02 15:39 ` Tom Rini
2024-09-10 18:41 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-09-02 15:39 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 3212 bytes --]
On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > >
> > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > system version. Add a special option to skip this build. This can be
> > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > tools/buildman/control.py | 3 ++-
> > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > >
> > > > > We should probably do this more generically, outside of buildman. We
> > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > (and we just need to define whatever the minimum version is), then we
> > > > > build our (not currently that new anymore) dtc instead.
> > > >
> > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > >
> > > > I'd be very happy for it to be applied as I think it is a better
> > > > solution than this one.
> > > >
> > > > I see that some poor sod tried to do this in Linux this morning.
> > >
> > > Any thoughts on that patch?
> >
> > I'm open to re-considering [1] again, but we need to handle the warning
> > problem first. That means...
> >
> > > Also I do see one problem. Newer dtc version produce a lot of
> > > warnings, which causes CI to fail. So if we always use the newest
> > > version, people are going to see a ton of warnings when they run
> > > locally. Am I missing something here?
> >
> > Well, it would be great to get our Kbuild logic anywhere close to
> > in-sync again with upstream. But syncing up the disabling warning flags
> > shouldn't be too hard.
>
> So, coming back to this patch, the nice thing about it is that it is
> deterministic. So people who build U-Boot and don't want funky
> behaviour will be happy. It will use the internal dtc by default. To
> use the external one, you must provide an option.
>
> This patch only affects buildman, but as you can see the mechanism it
> uses is to set the DTC variable, which people can do without buildman.
> It's just a convenience, but useful enough to have a flag, I believe.
Wait, that's right, we have DTC as a thing that can be set in the
environment, so why do we need something for buildman at all?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-02 15:39 ` Tom Rini
@ 2024-09-10 18:41 ` Simon Glass
2024-09-10 18:42 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-09-10 18:41 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > >
> > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > build our (not currently that new anymore) dtc instead.
> > > > >
> > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > >
> > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > solution than this one.
> > > > >
> > > > > I see that some poor sod tried to do this in Linux this morning.
> > > >
> > > > Any thoughts on that patch?
> > >
> > > I'm open to re-considering [1] again, but we need to handle the warning
> > > problem first. That means...
> > >
> > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > warnings, which causes CI to fail. So if we always use the newest
> > > > version, people are going to see a ton of warnings when they run
> > > > locally. Am I missing something here?
> > >
> > > Well, it would be great to get our Kbuild logic anywhere close to
> > > in-sync again with upstream. But syncing up the disabling warning flags
> > > shouldn't be too hard.
> >
> > So, coming back to this patch, the nice thing about it is that it is
> > deterministic. So people who build U-Boot and don't want funky
> > behaviour will be happy. It will use the internal dtc by default. To
> > use the external one, you must provide an option.
> >
> > This patch only affects buildman, but as you can see the mechanism it
> > uses is to set the DTC variable, which people can do without buildman.
> > It's just a convenience, but useful enough to have a flag, I believe.
>
> Wait, that's right, we have DTC as a thing that can be set in the
> environment, so why do we need something for buildman at all?
It's a convenience. I have found that it speeds things up quite a bit,
so I want to enable it most of the time. But we can't do it by default
and need it to be optional, I believe.
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-10 18:41 ` Simon Glass
@ 2024-09-10 18:42 ` Tom Rini
2024-09-10 18:45 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-09-10 18:42 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]
On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > >
> > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > (no changes since v1)
> > > > > > > >
> > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > >
> > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > >
> > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > solution than this one.
> > > > > >
> > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > >
> > > > > Any thoughts on that patch?
> > > >
> > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > problem first. That means...
> > > >
> > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > version, people are going to see a ton of warnings when they run
> > > > > locally. Am I missing something here?
> > > >
> > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > shouldn't be too hard.
> > >
> > > So, coming back to this patch, the nice thing about it is that it is
> > > deterministic. So people who build U-Boot and don't want funky
> > > behaviour will be happy. It will use the internal dtc by default. To
> > > use the external one, you must provide an option.
> > >
> > > This patch only affects buildman, but as you can see the mechanism it
> > > uses is to set the DTC variable, which people can do without buildman.
> > > It's just a convenience, but useful enough to have a flag, I believe.
> >
> > Wait, that's right, we have DTC as a thing that can be set in the
> > environment, so why do we need something for buildman at all?
>
> It's a convenience. I have found that it speeds things up quite a bit,
> so I want to enable it most of the time. But we can't do it by default
> and need it to be optional, I believe.
I wasn't clear, sorry. Why is:
$ export DTC=/usr/local/bin/dtc
$ tools/buildman/buildman ...
Enough?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-10 18:42 ` Tom Rini
@ 2024-09-10 18:45 ` Simon Glass
2024-09-10 18:52 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-09-10 18:45 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > (no changes since v1)
> > > > > > > > >
> > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > >
> > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > >
> > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > solution than this one.
> > > > > > >
> > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > >
> > > > > > Any thoughts on that patch?
> > > > >
> > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > problem first. That means...
> > > > >
> > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > version, people are going to see a ton of warnings when they run
> > > > > > locally. Am I missing something here?
> > > > >
> > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > shouldn't be too hard.
> > > >
> > > > So, coming back to this patch, the nice thing about it is that it is
> > > > deterministic. So people who build U-Boot and don't want funky
> > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > use the external one, you must provide an option.
> > > >
> > > > This patch only affects buildman, but as you can see the mechanism it
> > > > uses is to set the DTC variable, which people can do without buildman.
> > > > It's just a convenience, but useful enough to have a flag, I believe.
> > >
> > > Wait, that's right, we have DTC as a thing that can be set in the
> > > environment, so why do we need something for buildman at all?
> >
> > It's a convenience. I have found that it speeds things up quite a bit,
> > so I want to enable it most of the time. But we can't do it by default
> > and need it to be optional, I believe.
>
> I wasn't clear, sorry. Why is:
> $ export DTC=/usr/local/bin/dtc
> $ tools/buildman/buildman ...
>
> Enough?
My patch is more equivalent to:
DTC=`which dtc` buildman...
As I said it is a convenience which I want to use all the time,
including in CI. Are you worried about changing buildman? What is the
issue here?
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-10 18:45 ` Simon Glass
@ 2024-09-10 18:52 ` Tom Rini
2024-09-10 20:14 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-09-10 18:52 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 5256 bytes --]
On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > (no changes since v1)
> > > > > > > > > >
> > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > >
> > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > >
> > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > solution than this one.
> > > > > > > >
> > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > >
> > > > > > > Any thoughts on that patch?
> > > > > >
> > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > problem first. That means...
> > > > > >
> > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > locally. Am I missing something here?
> > > > > >
> > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > shouldn't be too hard.
> > > > >
> > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > use the external one, you must provide an option.
> > > > >
> > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > >
> > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > environment, so why do we need something for buildman at all?
> > >
> > > It's a convenience. I have found that it speeds things up quite a bit,
> > > so I want to enable it most of the time. But we can't do it by default
> > > and need it to be optional, I believe.
> >
> > I wasn't clear, sorry. Why is:
> > $ export DTC=/usr/local/bin/dtc
> > $ tools/buildman/buildman ...
> >
> > Enough?
>
> My patch is more equivalent to:
>
> DTC=`which dtc` buildman...
>
> As I said it is a convenience which I want to use all the time,
> including in CI. Are you worried about changing buildman? What is the
> issue here?
My question / concern is why do we need:
> 6 files changed, 66 insertions(+), 4 deletions(-)
When we can do it with zero code changes? And then similar to the
worries I set aside with the "venv" related changes, you're making one
tool be clever and work as exactly as expected (but perhaps in a "do
what I meant, not what I said" manner?) way. If we have "set DTC in
environment, it's always obeyed, can be passed directly to make" but
also "pass buildman a flag to say where dtc is" why do we need the
latter if it should already be working, and I think already is working?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-10 18:52 ` Tom Rini
@ 2024-09-10 20:14 ` Simon Glass
2024-09-10 22:07 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-09-10 20:14 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > >
> > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > (no changes since v1)
> > > > > > > > > > >
> > > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > >
> > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > >
> > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > solution than this one.
> > > > > > > > >
> > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > >
> > > > > > > > Any thoughts on that patch?
> > > > > > >
> > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > problem first. That means...
> > > > > > >
> > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > locally. Am I missing something here?
> > > > > > >
> > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > shouldn't be too hard.
> > > > > >
> > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > use the external one, you must provide an option.
> > > > > >
> > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > >
> > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > environment, so why do we need something for buildman at all?
> > > >
> > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > so I want to enable it most of the time. But we can't do it by default
> > > > and need it to be optional, I believe.
> > >
> > > I wasn't clear, sorry. Why is:
> > > $ export DTC=/usr/local/bin/dtc
> > > $ tools/buildman/buildman ...
> > >
> > > Enough?
> >
> > My patch is more equivalent to:
> >
> > DTC=`which dtc` buildman...
> >
> > As I said it is a convenience which I want to use all the time,
> > including in CI. Are you worried about changing buildman? What is the
> > issue here?
>
> My question / concern is why do we need:
>
> > 6 files changed, 66 insertions(+), 4 deletions(-)
>
> When we can do it with zero code changes?
We don't need anything here...but this helps with my productivity as
and can add this flag easily when doing large builds.
Note that half of those lines are a test and another half are just
plumbing it through all the layers. Any new flag will end up with
this, even if it is a few lines of 'real' code.
> And then similar to the
> worries I set aside with the "venv" related changes,
That change does fix a bug. Without the change, we cannot run code
coverage in CI, which I very-much want to turn on once Marek can
figure out the missing Binman tests.
> you're making one
> tool be clever and work as exactly as expected (but perhaps in a "do
> what I meant, not what I said" manner?) way.
The only magic is that it locates dtc on the path, but that is the
thing that I find valuable.
> If we have "set DTC in
> environment, it's always obeyed, can be passed directly to make" but
> also "pass buildman a flag to say where dtc is" why do we need the
> latter if it should already be working, and I think already is working?
The flag doesn't tell buildman where dtc is, though. It tells buildman
to hunt for it and use that in preference to the internal build. I am
not suggesting adding anything non-deterministics.
A general point here...buildman is pretty stable but it is not perfect
and we continue to tweak it to help make it a better tool. When it is
perfect, I will certainly stop sending patches for it. I normally only
send a patch when a bug is reported or something annoys me. The dtc
thing annoys me *a lot* because it chews up ~20% of my CPU for no
purpose. I had not realised how bad this problem was.
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-10 20:14 ` Simon Glass
@ 2024-09-10 22:07 ` Tom Rini
2024-09-12 1:01 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-09-10 22:07 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 7336 bytes --]
On Tue, Sep 10, 2024 at 02:14:35PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > > >
> > > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > > (no changes since v1)
> > > > > > > > > > > >
> > > > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > > >
> > > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > > >
> > > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > > solution than this one.
> > > > > > > > > >
> > > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > > >
> > > > > > > > > Any thoughts on that patch?
> > > > > > > >
> > > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > > problem first. That means...
> > > > > > > >
> > > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > > locally. Am I missing something here?
> > > > > > > >
> > > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > > shouldn't be too hard.
> > > > > > >
> > > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > > use the external one, you must provide an option.
> > > > > > >
> > > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > > >
> > > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > > environment, so why do we need something for buildman at all?
> > > > >
> > > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > > so I want to enable it most of the time. But we can't do it by default
> > > > > and need it to be optional, I believe.
> > > >
> > > > I wasn't clear, sorry. Why is:
> > > > $ export DTC=/usr/local/bin/dtc
> > > > $ tools/buildman/buildman ...
> > > >
> > > > Enough?
> > >
> > > My patch is more equivalent to:
> > >
> > > DTC=`which dtc` buildman...
> > >
> > > As I said it is a convenience which I want to use all the time,
> > > including in CI. Are you worried about changing buildman? What is the
> > > issue here?
> >
> > My question / concern is why do we need:
> >
> > > 6 files changed, 66 insertions(+), 4 deletions(-)
> >
> > When we can do it with zero code changes?
>
> We don't need anything here...but this helps with my productivity as
> and can add this flag easily when doing large builds.
>
> Note that half of those lines are a test and another half are just
> plumbing it through all the layers. Any new flag will end up with
> this, even if it is a few lines of 'real' code.
>
> > And then similar to the
> > worries I set aside with the "venv" related changes,
>
> That change does fix a bug. Without the change, we cannot run code
> coverage in CI, which I very-much want to turn on once Marek can
> figure out the missing Binman tests.
>
> > you're making one
> > tool be clever and work as exactly as expected (but perhaps in a "do
> > what I meant, not what I said" manner?) way.
>
> The only magic is that it locates dtc on the path, but that is the
> thing that I find valuable.
>
> > If we have "set DTC in
> > environment, it's always obeyed, can be passed directly to make" but
> > also "pass buildman a flag to say where dtc is" why do we need the
> > latter if it should already be working, and I think already is working?
>
> The flag doesn't tell buildman where dtc is, though. It tells buildman
> to hunt for it and use that in preference to the internal build. I am
> not suggesting adding anything non-deterministics.
>
> A general point here...buildman is pretty stable but it is not perfect
> and we continue to tweak it to help make it a better tool. When it is
> perfect, I will certainly stop sending patches for it. I normally only
> send a patch when a bug is reported or something annoys me. The dtc
> thing annoys me *a lot* because it chews up ~20% of my CPU for no
> purpose. I had not realised how bad this problem was.
Thanks for explaining. Whenever you want to pick this one up, go ahead
and send it along then. And I guess at some point we should look at (a)
re-syncing our libfdt code and (b) switching to using system dtc by
default?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-10 22:07 ` Tom Rini
@ 2024-09-12 1:01 ` Simon Glass
2024-09-12 17:44 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-09-12 1:01 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Tue, 10 Sept 2024 at 16:07, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Sep 10, 2024 at 02:14:35PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Tom,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >
> > > > > > > > > > > > > (no changes since v1)
> > > > > > > > > > > > >
> > > > > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > > > >
> > > > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > > > >
> > > > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > > > solution than this one.
> > > > > > > > > > >
> > > > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > > > >
> > > > > > > > > > Any thoughts on that patch?
> > > > > > > > >
> > > > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > > > problem first. That means...
> > > > > > > > >
> > > > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > > > locally. Am I missing something here?
> > > > > > > > >
> > > > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > > > shouldn't be too hard.
> > > > > > > >
> > > > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > > > use the external one, you must provide an option.
> > > > > > > >
> > > > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > > > >
> > > > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > > > environment, so why do we need something for buildman at all?
> > > > > >
> > > > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > > > so I want to enable it most of the time. But we can't do it by default
> > > > > > and need it to be optional, I believe.
> > > > >
> > > > > I wasn't clear, sorry. Why is:
> > > > > $ export DTC=/usr/local/bin/dtc
> > > > > $ tools/buildman/buildman ...
> > > > >
> > > > > Enough?
> > > >
> > > > My patch is more equivalent to:
> > > >
> > > > DTC=`which dtc` buildman...
> > > >
> > > > As I said it is a convenience which I want to use all the time,
> > > > including in CI. Are you worried about changing buildman? What is the
> > > > issue here?
> > >
> > > My question / concern is why do we need:
> > >
> > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > >
> > > When we can do it with zero code changes?
> >
> > We don't need anything here...but this helps with my productivity as
> > and can add this flag easily when doing large builds.
> >
> > Note that half of those lines are a test and another half are just
> > plumbing it through all the layers. Any new flag will end up with
> > this, even if it is a few lines of 'real' code.
> >
> > > And then similar to the
> > > worries I set aside with the "venv" related changes,
> >
> > That change does fix a bug. Without the change, we cannot run code
> > coverage in CI, which I very-much want to turn on once Marek can
> > figure out the missing Binman tests.
> >
> > > you're making one
> > > tool be clever and work as exactly as expected (but perhaps in a "do
> > > what I meant, not what I said" manner?) way.
> >
> > The only magic is that it locates dtc on the path, but that is the
> > thing that I find valuable.
> >
> > > If we have "set DTC in
> > > environment, it's always obeyed, can be passed directly to make" but
> > > also "pass buildman a flag to say where dtc is" why do we need the
> > > latter if it should already be working, and I think already is working?
> >
> > The flag doesn't tell buildman where dtc is, though. It tells buildman
> > to hunt for it and use that in preference to the internal build. I am
> > not suggesting adding anything non-deterministics.
> >
> > A general point here...buildman is pretty stable but it is not perfect
> > and we continue to tweak it to help make it a better tool. When it is
> > perfect, I will certainly stop sending patches for it. I normally only
> > send a patch when a bug is reported or something annoys me. The dtc
> > thing annoys me *a lot* because it chews up ~20% of my CPU for no
> > purpose. I had not realised how bad this problem was.
>
> Thanks for explaining. Whenever you want to pick this one up, go ahead
> and send it along then. And I guess at some point we should look at (a)
> re-syncing our libfdt code and (b) switching to using system dtc by
> default?
OK thank you. Perhaps we should file an issue for that? I am trying to
do one issue a week, although I have failed the last few weeks.
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-12 1:01 ` Simon Glass
@ 2024-09-12 17:44 ` Tom Rini
2024-09-19 14:13 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-09-12 17:44 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
[-- Attachment #1: Type: text/plain, Size: 8248 bytes --]
On Wed, Sep 11, 2024 at 07:01:37PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Tue, 10 Sept 2024 at 16:07, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 02:14:35PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > > > > Hi Tom,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > (no changes since v1)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > > > > >
> > > > > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > > > > >
> > > > > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > > > > solution than this one.
> > > > > > > > > > > >
> > > > > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > > > > >
> > > > > > > > > > > Any thoughts on that patch?
> > > > > > > > > >
> > > > > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > > > > problem first. That means...
> > > > > > > > > >
> > > > > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > > > > locally. Am I missing something here?
> > > > > > > > > >
> > > > > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > > > > shouldn't be too hard.
> > > > > > > > >
> > > > > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > > > > use the external one, you must provide an option.
> > > > > > > > >
> > > > > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > > > > >
> > > > > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > > > > environment, so why do we need something for buildman at all?
> > > > > > >
> > > > > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > > > > so I want to enable it most of the time. But we can't do it by default
> > > > > > > and need it to be optional, I believe.
> > > > > >
> > > > > > I wasn't clear, sorry. Why is:
> > > > > > $ export DTC=/usr/local/bin/dtc
> > > > > > $ tools/buildman/buildman ...
> > > > > >
> > > > > > Enough?
> > > > >
> > > > > My patch is more equivalent to:
> > > > >
> > > > > DTC=`which dtc` buildman...
> > > > >
> > > > > As I said it is a convenience which I want to use all the time,
> > > > > including in CI. Are you worried about changing buildman? What is the
> > > > > issue here?
> > > >
> > > > My question / concern is why do we need:
> > > >
> > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > >
> > > > When we can do it with zero code changes?
> > >
> > > We don't need anything here...but this helps with my productivity as
> > > and can add this flag easily when doing large builds.
> > >
> > > Note that half of those lines are a test and another half are just
> > > plumbing it through all the layers. Any new flag will end up with
> > > this, even if it is a few lines of 'real' code.
> > >
> > > > And then similar to the
> > > > worries I set aside with the "venv" related changes,
> > >
> > > That change does fix a bug. Without the change, we cannot run code
> > > coverage in CI, which I very-much want to turn on once Marek can
> > > figure out the missing Binman tests.
> > >
> > > > you're making one
> > > > tool be clever and work as exactly as expected (but perhaps in a "do
> > > > what I meant, not what I said" manner?) way.
> > >
> > > The only magic is that it locates dtc on the path, but that is the
> > > thing that I find valuable.
> > >
> > > > If we have "set DTC in
> > > > environment, it's always obeyed, can be passed directly to make" but
> > > > also "pass buildman a flag to say where dtc is" why do we need the
> > > > latter if it should already be working, and I think already is working?
> > >
> > > The flag doesn't tell buildman where dtc is, though. It tells buildman
> > > to hunt for it and use that in preference to the internal build. I am
> > > not suggesting adding anything non-deterministics.
> > >
> > > A general point here...buildman is pretty stable but it is not perfect
> > > and we continue to tweak it to help make it a better tool. When it is
> > > perfect, I will certainly stop sending patches for it. I normally only
> > > send a patch when a bug is reported or something annoys me. The dtc
> > > thing annoys me *a lot* because it chews up ~20% of my CPU for no
> > > purpose. I had not realised how bad this problem was.
> >
> > Thanks for explaining. Whenever you want to pick this one up, go ahead
> > and send it along then. And I guess at some point we should look at (a)
> > re-syncing our libfdt code and (b) switching to using system dtc by
> > default?
>
> OK thank you. Perhaps we should file an issue for that? I am trying to
> do one issue a week, although I have failed the last few weeks.
Can't hurt, yeah.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-12 17:44 ` Tom Rini
@ 2024-09-19 14:13 ` Simon Glass
2024-09-26 22:07 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-09-19 14:13 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt
Hi Tom,
On Thu, 12 Sept 2024 at 19:44, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Sep 11, 2024 at 07:01:37PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 10 Sept 2024 at 16:07, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Sep 10, 2024 at 02:14:35PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > (no changes since v1)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > > > > > solution than this one.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > > > > > >
> > > > > > > > > > > > Any thoughts on that patch?
> > > > > > > > > > >
> > > > > > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > > > > > problem first. That means...
> > > > > > > > > > >
> > > > > > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > > > > > locally. Am I missing something here?
> > > > > > > > > > >
> > > > > > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > > > > > shouldn't be too hard.
> > > > > > > > > >
> > > > > > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > > > > > use the external one, you must provide an option.
> > > > > > > > > >
> > > > > > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > > > > > >
> > > > > > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > > > > > environment, so why do we need something for buildman at all?
> > > > > > > >
> > > > > > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > > > > > so I want to enable it most of the time. But we can't do it by default
> > > > > > > > and need it to be optional, I believe.
> > > > > > >
> > > > > > > I wasn't clear, sorry. Why is:
> > > > > > > $ export DTC=/usr/local/bin/dtc
> > > > > > > $ tools/buildman/buildman ...
> > > > > > >
> > > > > > > Enough?
> > > > > >
> > > > > > My patch is more equivalent to:
> > > > > >
> > > > > > DTC=`which dtc` buildman...
> > > > > >
> > > > > > As I said it is a convenience which I want to use all the time,
> > > > > > including in CI. Are you worried about changing buildman? What is the
> > > > > > issue here?
> > > > >
> > > > > My question / concern is why do we need:
> > > > >
> > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > >
> > > > > When we can do it with zero code changes?
> > > >
> > > > We don't need anything here...but this helps with my productivity as
> > > > and can add this flag easily when doing large builds.
> > > >
> > > > Note that half of those lines are a test and another half are just
> > > > plumbing it through all the layers. Any new flag will end up with
> > > > this, even if it is a few lines of 'real' code.
> > > >
> > > > > And then similar to the
> > > > > worries I set aside with the "venv" related changes,
> > > >
> > > > That change does fix a bug. Without the change, we cannot run code
> > > > coverage in CI, which I very-much want to turn on once Marek can
> > > > figure out the missing Binman tests.
> > > >
> > > > > you're making one
> > > > > tool be clever and work as exactly as expected (but perhaps in a "do
> > > > > what I meant, not what I said" manner?) way.
> > > >
> > > > The only magic is that it locates dtc on the path, but that is the
> > > > thing that I find valuable.
> > > >
> > > > > If we have "set DTC in
> > > > > environment, it's always obeyed, can be passed directly to make" but
> > > > > also "pass buildman a flag to say where dtc is" why do we need the
> > > > > latter if it should already be working, and I think already is working?
> > > >
> > > > The flag doesn't tell buildman where dtc is, though. It tells buildman
> > > > to hunt for it and use that in preference to the internal build. I am
> > > > not suggesting adding anything non-deterministics.
> > > >
> > > > A general point here...buildman is pretty stable but it is not perfect
> > > > and we continue to tweak it to help make it a better tool. When it is
> > > > perfect, I will certainly stop sending patches for it. I normally only
> > > > send a patch when a bug is reported or something annoys me. The dtc
> > > > thing annoys me *a lot* because it chews up ~20% of my CPU for no
> > > > purpose. I had not realised how bad this problem was.
> > >
> > > Thanks for explaining. Whenever you want to pick this one up, go ahead
> > > and send it along then. And I guess at some point we should look at (a)
> > > re-syncing our libfdt code and (b) switching to using system dtc by
> > > default?
> >
> > OK thank you. Perhaps we should file an issue for that? I am trying to
> > do one issue a week, although I have failed the last few weeks.
>
> Can't hurt, yeah.
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/24
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/25
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/3] buildman: Allow skipping the dtc build
2024-09-19 14:13 ` Simon Glass
@ 2024-09-26 22:07 ` Simon Glass
0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2024-09-26 22:07 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Brandon Maier,
Heinrich Schuchardt, Tom Rini
Hi Tom,
On Thu, 12 Sept 2024 at 19:44, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Sep 11, 2024 at 07:01:37PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 10 Sept 2024 at 16:07, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Sep 10, 2024 at 02:14:35PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > (no changes since v1)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > tools/buildman/builder.py | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > > > > > > tools/buildman/builderthread.py | 4 ++--
> > > > > > > > > > > > > > > tools/buildman/buildman.rst | 3 +++
> > > > > > > > > > > > > > > tools/buildman/cmdline.py | 2 ++
> > > > > > > > > > > > > > > tools/buildman/control.py | 3 ++-
> > > > > > > > > > > > > > > tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > > > > > solution than this one.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > > > > > >
> > > > > > > > > > > > Any thoughts on that patch?
> > > > > > > > > > >
> > > > > > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > > > > > problem first. That means...
> > > > > > > > > > >
> > > > > > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > > > > > locally. Am I missing something here?
> > > > > > > > > > >
> > > > > > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > > > > > shouldn't be too hard.
> > > > > > > > > >
> > > > > > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > > > > > use the external one, you must provide an option.
> > > > > > > > > >
> > > > > > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > > > > > >
> > > > > > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > > > > > environment, so why do we need something for buildman at all?
> > > > > > > >
> > > > > > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > > > > > so I want to enable it most of the time. But we can't do it by default
> > > > > > > > and need it to be optional, I believe.
> > > > > > >
> > > > > > > I wasn't clear, sorry. Why is:
> > > > > > > $ export DTC=/usr/local/bin/dtc
> > > > > > > $ tools/buildman/buildman ...
> > > > > > >
> > > > > > > Enough?
> > > > > >
> > > > > > My patch is more equivalent to:
> > > > > >
> > > > > > DTC=`which dtc` buildman...
> > > > > >
> > > > > > As I said it is a convenience which I want to use all the time,
> > > > > > including in CI. Are you worried about changing buildman? What is the
> > > > > > issue here?
> > > > >
> > > > > My question / concern is why do we need:
> > > > >
> > > > > > 6 files changed, 66 insertions(+), 4 deletions(-)
> > > > >
> > > > > When we can do it with zero code changes?
> > > >
> > > > We don't need anything here...but this helps with my productivity as
> > > > and can add this flag easily when doing large builds.
> > > >
> > > > Note that half of those lines are a test and another half are just
> > > > plumbing it through all the layers. Any new flag will end up with
> > > > this, even if it is a few lines of 'real' code.
> > > >
> > > > > And then similar to the
> > > > > worries I set aside with the "venv" related changes,
> > > >
> > > > That change does fix a bug. Without the change, we cannot run code
> > > > coverage in CI, which I very-much want to turn on once Marek can
> > > > figure out the missing Binman tests.
> > > >
> > > > > you're making one
> > > > > tool be clever and work as exactly as expected (but perhaps in a "do
> > > > > what I meant, not what I said" manner?) way.
> > > >
> > > > The only magic is that it locates dtc on the path, but that is the
> > > > thing that I find valuable.
> > > >
> > > > > If we have "set DTC in
> > > > > environment, it's always obeyed, can be passed directly to make" but
> > > > > also "pass buildman a flag to say where dtc is" why do we need the
> > > > > latter if it should already be working, and I think already is working?
> > > >
> > > > The flag doesn't tell buildman where dtc is, though. It tells buildman
> > > > to hunt for it and use that in preference to the internal build. I am
> > > > not suggesting adding anything non-deterministics.
> > > >
> > > > A general point here...buildman is pretty stable but it is not perfect
> > > > and we continue to tweak it to help make it a better tool. When it is
> > > > perfect, I will certainly stop sending patches for it. I normally only
> > > > send a patch when a bug is reported or something annoys me. The dtc
> > > > thing annoys me *a lot* because it chews up ~20% of my CPU for no
> > > > purpose. I had not realised how bad this problem was.
> > >
> > > Thanks for explaining. Whenever you want to pick this one up, go ahead
> > > and send it along then. And I guess at some point we should look at (a)
> > > re-syncing our libfdt code and (b) switching to using system dtc by
> > > default?
> >
> > OK thank you. Perhaps we should file an issue for that? I am trying to
> > do one issue a week, although I have failed the last few weeks.
>
> Can't hurt, yeah.
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/24
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/25
Regards,
Simon
Applied to u-boot-dm/next, thanks!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds
2024-08-15 19:57 [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in Simon Glass
2024-08-15 19:57 ` [PATCH v3 1/3] buildman: Support building within a Python venv Simon Glass
2024-08-15 19:57 ` [PATCH v3 2/3] buildman: Allow skipping the dtc build Simon Glass
@ 2024-08-15 19:57 ` Simon Glass
2024-10-10 18:50 ` Simon Glass
2024-09-06 22:37 ` (subset) [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in Tom Rini
3 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-08-15 19:57 UTC (permalink / raw)
To: U-Boot Mailing List
Cc: Tom Rini, Simon Glass, Andrejs Cainikovs, Jiaxun Yang,
Marek Vasut, Sean Anderson
This is quite a significant waste of time in the world builds as dtc is
built separately for each board. Skip this to speed up the builds.
Unfortunately the newer dtc produces a lot of warnings, which causes CI
to fail. I am not sure what we can do about this.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Drop patches previously applied
- Add dtc patches
.azure-pipelines.yml | 2 +-
.gitlab-ci.yml | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
index e1b2f87b974..4bdf1daa59c 100644
--- a/.azure-pipelines.yml
+++ b/.azure-pipelines.yml
@@ -561,7 +561,7 @@ stages:
cat << "EOF" >> build.sh
if [[ "${BUILDMAN}" != "" ]]; then
ret=0;
- tools/buildman/buildman -o /tmp -PEWM ${BUILDMAN} ${OVERRIDE} || ret=$?;
+ tools/buildman/buildman -o /tmp -PEWM --dtc-skip ${BUILDMAN} ${OVERRIDE} || ret=$?;
if [[ $ret -ne 0 ]]; then
tools/buildman/buildman -o /tmp -seP ${BUILDMAN};
exit $ret;
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 0a15b7352cd..b231b6cd5a4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -104,7 +104,7 @@ build all 32bit ARM platforms:
- ret=0;
git config --global --add safe.directory "${CI_PROJECT_DIR}";
pip install -r tools/buildman/requirements.txt;
- ./tools/buildman/buildman -o /tmp -PEWM arm -x aarch64 || ret=$?;
+ ./tools/buildman/buildman -o /tmp -PEWM --dtc-skip arm -x aarch64 || ret=$?;
if [[ $ret -ne 0 ]]; then
./tools/buildman/buildman -o /tmp -seP;
exit $ret;
@@ -118,7 +118,7 @@ build all 64bit ARM platforms:
- ret=0;
git config --global --add safe.directory "${CI_PROJECT_DIR}";
pip install -r tools/buildman/requirements.txt;
- ./tools/buildman/buildman -o /tmp -PEWM aarch64 || ret=$?;
+ ./tools/buildman/buildman -o /tmp -PEWM --dtc-skip aarch64 || ret=$?;
if [[ $ret -ne 0 ]]; then
./tools/buildman/buildman -o /tmp -seP;
exit $ret;
@@ -129,7 +129,7 @@ build all PowerPC platforms:
script:
- ret=0;
git config --global --add safe.directory "${CI_PROJECT_DIR}";
- ./tools/buildman/buildman -o /tmp -P -E -W powerpc || ret=$?;
+ ./tools/buildman/buildman -o /tmp -PEW --dtc-skip powerpc || ret=$?;
if [[ $ret -ne 0 ]]; then
./tools/buildman/buildman -o /tmp -seP;
exit $ret;
@@ -140,7 +140,7 @@ build all other platforms:
script:
- ret=0;
git config --global --add safe.directory "${CI_PROJECT_DIR}";
- ./tools/buildman/buildman -o /tmp -PEWM -x arm,powerpc || ret=$?;
+ ./tools/buildman/buildman -o /tmp -PEWM --dtc-skip -x arm,powerpc || ret=$?;
if [[ $ret -ne 0 ]]; then
./tools/buildman/buildman -o /tmp -seP;
exit $ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds
2024-08-15 19:57 ` [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds Simon Glass
@ 2024-10-10 18:50 ` Simon Glass
2024-10-10 19:01 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-10-10 18:50 UTC (permalink / raw)
To: U-Boot Mailing List
Cc: Tom Rini, Andrejs Cainikovs, Jiaxun Yang, Marek Vasut,
Sean Anderson
Hi Tom,
On Thu, 15 Aug 2024 at 13:58, Simon Glass <sjg@chromium.org> wrote:
>
> This is quite a significant waste of time in the world builds as dtc is
> built separately for each board. Skip this to speed up the builds.
>
> Unfortunately the newer dtc produces a lot of warnings, which causes CI
> to fail. I am not sure what we can do about this.
What do you think about this one?
We could install an older dtc in the container, perhaps?
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Drop patches previously applied
> - Add dtc patches
>
> .azure-pipelines.yml | 2 +-
> .gitlab-ci.yml | 8 ++++----
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> index e1b2f87b974..4bdf1daa59c 100644
> --- a/.azure-pipelines.yml
> +++ b/.azure-pipelines.yml
> @@ -561,7 +561,7 @@ stages:
> cat << "EOF" >> build.sh
> if [[ "${BUILDMAN}" != "" ]]; then
> ret=0;
> - tools/buildman/buildman -o /tmp -PEWM ${BUILDMAN} ${OVERRIDE} || ret=$?;
> + tools/buildman/buildman -o /tmp -PEWM --dtc-skip ${BUILDMAN} ${OVERRIDE} || ret=$?;
> if [[ $ret -ne 0 ]]; then
> tools/buildman/buildman -o /tmp -seP ${BUILDMAN};
> exit $ret;
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 0a15b7352cd..b231b6cd5a4 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -104,7 +104,7 @@ build all 32bit ARM platforms:
> - ret=0;
> git config --global --add safe.directory "${CI_PROJECT_DIR}";
> pip install -r tools/buildman/requirements.txt;
> - ./tools/buildman/buildman -o /tmp -PEWM arm -x aarch64 || ret=$?;
> + ./tools/buildman/buildman -o /tmp -PEWM --dtc-skip arm -x aarch64 || ret=$?;
> if [[ $ret -ne 0 ]]; then
> ./tools/buildman/buildman -o /tmp -seP;
> exit $ret;
> @@ -118,7 +118,7 @@ build all 64bit ARM platforms:
> - ret=0;
> git config --global --add safe.directory "${CI_PROJECT_DIR}";
> pip install -r tools/buildman/requirements.txt;
> - ./tools/buildman/buildman -o /tmp -PEWM aarch64 || ret=$?;
> + ./tools/buildman/buildman -o /tmp -PEWM --dtc-skip aarch64 || ret=$?;
> if [[ $ret -ne 0 ]]; then
> ./tools/buildman/buildman -o /tmp -seP;
> exit $ret;
> @@ -129,7 +129,7 @@ build all PowerPC platforms:
> script:
> - ret=0;
> git config --global --add safe.directory "${CI_PROJECT_DIR}";
> - ./tools/buildman/buildman -o /tmp -P -E -W powerpc || ret=$?;
> + ./tools/buildman/buildman -o /tmp -PEW --dtc-skip powerpc || ret=$?;
> if [[ $ret -ne 0 ]]; then
> ./tools/buildman/buildman -o /tmp -seP;
> exit $ret;
> @@ -140,7 +140,7 @@ build all other platforms:
> script:
> - ret=0;
> git config --global --add safe.directory "${CI_PROJECT_DIR}";
> - ./tools/buildman/buildman -o /tmp -PEWM -x arm,powerpc || ret=$?;
> + ./tools/buildman/buildman -o /tmp -PEWM --dtc-skip -x arm,powerpc || ret=$?;
> if [[ $ret -ne 0 ]]; then
> ./tools/buildman/buildman -o /tmp -seP;
> exit $ret;
> --
> 2.34.1
>
Regards,
SImon
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds
2024-10-10 18:50 ` Simon Glass
@ 2024-10-10 19:01 ` Tom Rini
2024-10-14 19:13 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Tom Rini @ 2024-10-10 19:01 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Jiaxun Yang, Marek Vasut,
Sean Anderson
[-- Attachment #1: Type: text/plain, Size: 852 bytes --]
On Thu, Oct 10, 2024 at 12:50:11PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 15 Aug 2024 at 13:58, Simon Glass <sjg@chromium.org> wrote:
> >
> > This is quite a significant waste of time in the world builds as dtc is
> > built separately for each board. Skip this to speed up the builds.
> >
> > Unfortunately the newer dtc produces a lot of warnings, which causes CI
> > to fail. I am not sure what we can do about this.
>
> What do you think about this one?
>
> We could install an older dtc in the container, perhaps?
Is backporting applying whatever dtc flags we need to ignore problems
that Linux isn't enforcing so difficult? That would then tell us just
how much work it is / isn't to deal with the rest of the dts files (or
poke maintainers to switch to OF_UPSTREAM where the issues are likely
fixed).
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds
2024-10-10 19:01 ` Tom Rini
@ 2024-10-14 19:13 ` Simon Glass
2024-10-14 22:43 ` Tom Rini
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2024-10-14 19:13 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Andrejs Cainikovs, Jiaxun Yang, Marek Vasut,
Sean Anderson
Hi Tom,
On Thu, 10 Oct 2024 at 13:01, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Oct 10, 2024 at 12:50:11PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 15 Aug 2024 at 13:58, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > This is quite a significant waste of time in the world builds as dtc is
> > > built separately for each board. Skip this to speed up the builds.
> > >
> > > Unfortunately the newer dtc produces a lot of warnings, which causes CI
> > > to fail. I am not sure what we can do about this.
> >
> > What do you think about this one?
> >
> > We could install an older dtc in the container, perhaps?
>
> Is backporting applying whatever dtc flags we need to ignore problems
> that Linux isn't enforcing so difficult? That would then tell us just
> how much work it is / isn't to deal with the rest of the dts files (or
> poke maintainers to switch to OF_UPSTREAM where the issues are likely
> fixed).
That could work. Shall I give it a try and then we apply this patch if
I figure it out?
Regards,
Simon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds
2024-10-14 19:13 ` Simon Glass
@ 2024-10-14 22:43 ` Tom Rini
0 siblings, 0 replies; 27+ messages in thread
From: Tom Rini @ 2024-10-14 22:43 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Andrejs Cainikovs, Jiaxun Yang, Marek Vasut,
Sean Anderson
[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]
On Mon, Oct 14, 2024 at 01:13:26PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 10 Oct 2024 at 13:01, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 10, 2024 at 12:50:11PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 15 Aug 2024 at 13:58, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > This is quite a significant waste of time in the world builds as dtc is
> > > > built separately for each board. Skip this to speed up the builds.
> > > >
> > > > Unfortunately the newer dtc produces a lot of warnings, which causes CI
> > > > to fail. I am not sure what we can do about this.
> > >
> > > What do you think about this one?
> > >
> > > We could install an older dtc in the container, perhaps?
> >
> > Is backporting applying whatever dtc flags we need to ignore problems
> > that Linux isn't enforcing so difficult? That would then tell us just
> > how much work it is / isn't to deal with the rest of the dts files (or
> > poke maintainers to switch to OF_UPSTREAM where the issues are likely
> > fixed).
>
> That could work. Shall I give it a try and then we apply this patch if
> I figure it out?
Sure, thanks.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: (subset) [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in
2024-08-15 19:57 [PATCH v3 0/3] This series adds a few patches to make it easy to use the system dtc in Simon Glass
` (2 preceding siblings ...)
2024-08-15 19:57 ` [PATCH v3 3/3] RFC: CI: Skip building dtc for world builds Simon Glass
@ 2024-09-06 22:37 ` Tom Rini
3 siblings, 0 replies; 27+ messages in thread
From: Tom Rini @ 2024-09-06 22:37 UTC (permalink / raw)
To: U-Boot Mailing List, Simon Glass
Cc: Andrejs Cainikovs, Brandon Maier, Heinrich Schuchardt,
Jiaxun Yang, Marek Vasut, Quentin Schulz, Sean Anderson
On Thu, 15 Aug 2024 13:57:43 -0600, Simon Glass wrote:
> buildman. It updates CI to do this for world builds, to save time.
>
> It also includes an old patch which is now needed again since:
>
> e13fcae3fce Revert "buildman: Always use the full path in...
>
> Changes in v3:
> - Drop patches previously applied
> - Add dtc patches
>
> [...]
Applied to u-boot/next, thanks!
--
Tom
^ permalink raw reply [flat|nested] 27+ messages in thread