Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DAOS-16621 build: Fix Go versions in rpm/deb packaging #15174

Merged
merged 8 commits into from
Sep 30, 2024
Merged

DAOS-16621 build: Fix Go versions in rpm/deb packaging #15174

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a237458
DAOS-16621 build: Fix Go versions in rpm/deb packaging
kjacque Sep 23, 2024
f17c1d5
Address review comments
kjacque Sep 24, 2024
2942650
Update Ubuntu install script
kjacque Sep 24, 2024
408e2d4
Add gpg-agent to ubuntu install
kjacque Sep 25, 2024
2b4106c
Fix pylint error
kjacque Sep 25, 2024
95f6cd3
Harden minimum Go version detection
kjacque Sep 26, 2024
b1ee4cf
Address review comments
kjacque Sep 26, 2024
11ac499
Move add-apt-repository call to Dockerfile
kjacque Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions debian/control
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Build-Depends: debhelper (>= 10),
dpdk-dev (>= 21.11.2),
libisal-crypto-dev,
libcunit1-dev,
golang-go (>= 1.21),
golang-go (>= 2:1.21),
libboost-dev,
libspdk-dev (>= 22.01.2),
libipmctl-dev,
Expand Down Expand Up @@ -117,7 +117,7 @@ Depends: python (>=3.8), python3, python-yaml, python3-yaml,
${shlibs:Depends}, ${misc:Depends},
daos-client (= ${binary:Version}),
daos-admin (= ${binary:Version}),
golang-go (>=1.21),
golang-go (>= 2:1.21),
libcapstone-dev
Description: The Distributed Asynchronous Object Storage (DAOS) is an open-source
software-defined object store designed from the ground up for
Expand Down
25 changes: 24 additions & 1 deletion site_scons/site_tools/go_builder.py
View file
Open in desktop
kjacque marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import re
import subprocess # nosec B404

from SCons.Script import Configure, Exit, File, GetOption, Glob, Scanner
from SCons.Script import Configure, Dir, Exit, File, GetOption, Glob, Scanner

GO_COMPILER = 'go'
include_re = re.compile(r'\#include [<"](\S+[>"])', re.M)
Expand Down Expand Up @@ -48,6 +48,17 @@ def _scan_go_file(node, env, _path):
return includes


def get_min_go_version():
"""Get go minimum version from go.mod"""
go_mod_path = os.path.join(Dir('#').abspath, "src", "control", "go.mod")
with open(go_mod_path, 'r') as f:
for line in f:
if line.startswith('go '): # e.g. "go 1.21"
parts = line.split()
return parts[1] + ".0" # e.g. "1.21.0"
kjacque marked this conversation as resolved.
Show resolved Hide resolved
return None


def get_go_version(output):
"""Capture only the version after 'go'"""
ver_re = re.compile(r'go([0-9\.]+)')
Expand Down Expand Up @@ -80,6 +91,13 @@ def _check_go_version(context):
context.Result(0)
return 0

context.Display(f'Getting minimum {env.d_go_bin} version... ')
ashleypittman marked this conversation as resolved.
Show resolved Hide resolved
min_go_version = get_min_go_version()
ashleypittman marked this conversation as resolved.
Show resolved Hide resolved
if min_go_version is None:
context.Result('failed to extract minimum version from go.mod')
return 0
context.Display(min_go_version + '\n')

context.Display(f'Checking {env.d_go_bin} version... ')
cmd_rc = subprocess.run([env.d_go_bin, 'version'], check=True, stdout=subprocess.PIPE)
out = cmd_rc.stdout.decode('utf-8').strip()
Expand All @@ -92,6 +110,11 @@ def _check_go_version(context):
if go_version is None:
context.Result(f'failed to get version from "{out}"')
return 0
if len([x for x, y in zip(go_version.split('.'), min_go_version.split('.'))
if int(x) < int(y)]) > 0:
context.Result(f'{out} is too old (min supported: {min_go_version}) ')
return 0

context.Result(go_version)
return 1

Expand Down
7 changes: 4 additions & 3 deletions src/control/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
module github.com/daos-stack/daos/src/control

// NB: When updating minimum Go build version, don't forget to update rpm and debian packaging specs.
// NB: When updating minimum Go build version, don't forget to update:
// - rpm packaging version checks: utils/rpms/daos.spec
// - debian packaging version checks: debian/control
// Scons uses this file to extract the minimum version.
go 1.21

toolchain go1.22.3

Comment on lines -5 to -6
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with go, what does this part of the change do?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically like a requirements.txt file in python, specifies package versions to be pulled in when installing into a golang environment (usually installed into user home directory). I'm guessing the removal of 1.22.3 was just to relax requirements as 1.22 not available on all distributions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the go mod tools added that line automatically, but I don't think we need it. It seems that it's only relevant if we want to use a different toolchain version than the minimum go version.

require (
github.com/Jille/raft-grpc-transport v1.2.0
github.com/desertbit/grumble v1.1.3
Expand Down
2 changes: 1 addition & 1 deletion utils/rpms/daos.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ getent passwd daos_agent >/dev/null || useradd -s /sbin/nologin -r -g daos_agent
# No files in a shim package

%changelog
* Mon Sep 23 2024 Kris Jacque <kris.jacque@intel.com> 2.3.103-6
* Mon Sep 23 2024 Kris Jacque <kris.jacque@intel.com> 2.7.100-6
- Bump min supported go version to 1.21

* Thu Aug 15 2024 Michael MacDonald <mjmac@google.com> 2.7.100-5
Expand Down
5 changes: 5 additions & 0 deletions utils/scripts/install-ubuntu.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ set -e

arch=$(uname -i)

# DAOS requires newer golang version than the one available in core ubuntu repo
apt-get install gpg-agent software-properties-common
add-apt-repository ppa:longsleep/golang-backports
apt update
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought I'd made a comment on this change but I can't see it anywhere, apologies if I'm repeating myself here.

One of the intentions of these scripts is that an admin should be able to call them to install required packages and to do so safely, so therefore they install packages without the -y option for example. The package manager/repo configuration is all done in the dockerfile/helper scripts and this script then just installs the named packages.

This code breaks that model so the script will re-configure the node as well as install code and in addition it's also possibly not idempotent any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested moving the Dockerfile Ubuntu version forward to 24.04 LTS and ran into a warning with the interception library (src/client/dfuse/il), I think due to an upgraded version of gcc.

In file included from src/client/dfuse/il/int_posix.c:32:
src/client/dfuse/il/intercept.h:38:21: error: ‘fseek64’ specifies less restrictive attribute than its target ‘fseek’: ‘nonnull’ [-Werror=missing-attributes]
   38 |         ACTION(int, fseek, (FILE *, long, int))                                                    \
      |                     ^~~~~
src/client/dfuse/il/intercept.h:106:27: note: in definition of macro ‘IOIL_DECLARE_ALIAS64’
  106 |         DFUSE_PUBLIC type name##64 params __attribute__((weak, alias(#name)));
      |                           ^~~~
src/client/dfuse/il/int_posix.c:3023:1: note: in expansion of macro ‘FOREACH_ALIASED_INTERCEPT’
 3023 | FOREACH_ALIASED_INTERCEPT(IOIL_DECLARE_ALIAS64)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/client/dfuse/il/int_posix.c:21:
/usr/include/stdio.h:779:12: note: ‘fseek64’ target declared here
  779 | extern int fseek (FILE *__stream, long int __off, int __whence)
      |            ^~~~~
cc1: all warnings being treated as errors

I think pushing up the Ubuntu version is out of scope for this task, given that it'll require production code changes.

I could shift the PPA addition to the Dockerfile if that would be an acceptable compromise.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's build errors then we definitely should update, that's what we're looking for with this test. Outside the scope of this PR though but I'll file a ticket and take it on.


apt-get install \
autoconf \
build-essential \
Expand Down
Loading