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.

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kjacque
Copy link
Contributor

@kjacque kjacque commented Sep 23, 2024

  • Set Go minimum version to 1.21 in rpm and debian packaging spec files.
  • Remove scons Go version check, as it will be checked in Go build based on go.mod file.
  • Add a reminder in go.mod file so we remember the packaging files when bumping the minimum Go version in the future.

Required-githooks: true

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

- Set Go minimum version to 1.21 in rpm and debian packaging spec
  files.
- Remove scons Go version check, as it will be checked in Go build
  based on go.mod file.
- Add a reminder in go.mod file so we remember the packaging files
  when bumping the minimum Go version in the future.

Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
@kjacque kjacque self-assigned this Sep 23, 2024
@kjacque kjacque requested review from a team as code owners September 23, 2024 22:23
Copy link

github-actions bot commented Sep 23, 2024

Ticket title is 'Fix Go version checks in scons and packaging specfiles'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-16621

@kjacque
Copy link
Contributor Author

kjacque commented Sep 23, 2024

No need to run feature tests -- these changes affect the build only.

Copy link
Contributor

@ashleypittman ashleypittman left a comment

Choose a reason for hiding this comment

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

I don't know if you're aware but docker builds on Ubuntu are failing, as per the build log at https://github.com/daos-stack/daos/actions/runs/10962467337/job/30441777476

What is happening there is the default go version is 1.18, the scons check is detecting this and passing, then there's a non-obvious build failure later on. In light of that perhaps we need to keep the scons check for now (but update the target version obviously) but also we need to update the packages for that build somehow.

debian/control Outdated
@@ -25,7 +25,7 @@ Build-Depends: debhelper (>= 10),
dpdk-dev (>= 21.11.2),
libisal-crypto-dev,
libcunit1-dev,
golang-go (>= 1.18),
golang-go (>= 1.21),
Copy link
Contributor

Choose a reason for hiding this comment

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

This has come up before and got dropped but I think the golang-go version on debian uses an epoch so you need to specify the version as I've indicated here.

Suggested change
golang-go (>= 1.21),
golang-go (>= 2:1.21),

Copy link
Contributor

Choose a reason for hiding this comment

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

https://packages.debian.org/bookworm/golang-go for the upstream package, and yes it does have/require an epoch number.

site_scons/site_tools/go_builder.py Show resolved Hide resolved
@kjacque
Copy link
Contributor Author

kjacque commented Sep 24, 2024

In light of that perhaps we need to keep the scons check for now (but update the target version obviously) but also we need to update the packages for that build somehow.

Ubuntu LTS versions tend to stick at a very old version of Go, to the point where I think it's more common to either download/install Go manually or use a third-party PPA. The longsleep/go-backports PPA is the one I've seen used most often, and is updated on a regular cadence. Do we need to get approval for that? @brianjmurrell what do you think?

@mjmac
Copy link
Contributor

mjmac commented Sep 24, 2024

Ubuntu LTS versions tend to stick at a very old version of Go, to the point where I think it's more common to either download/install Go manually or use a third-party PPA. The longsleep/go-backports PPA is the one I've seen used most often, and is updated on a regular cadence. Do we need to get approval for that? @brianjmurrell what do you think?

A better option than the PPA is to just install the official golang-go packages from the next LTS release. There is some discussion about the best way to do this in another PR: https://github.com/daos-stack/daos/pull/15150/files#diff-44f4cb066190d53259ef4c6c84dbac0f295f7e229ddb213000deadb5b9f7cb9eR17

It's worth noting that 22.04 is not the current LTS version of Ubuntu anymore. 24.04 was released this past April. We may not want to take on updating the version of Ubuntu being installed as part of this PR, but it should probably be scoped for the 2.8 timeframe.

utils/rpms/daos.spec Outdated Show resolved Hide resolved
site_scons/site_tools/go_builder.py Show resolved Hide resolved
- Fix debian Go versions
- In scons, get minimum Go version from go.mod
- Add reminder comment in go.mod
- Fix changelog version in rpm specfile
- Use longsleep/go-backports repo for Go versions in Dockerfile

Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
It's needed to install an external PPA.

Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
@kjacque
Copy link
Contributor Author

kjacque commented Sep 25, 2024

Ubuntu LTS versions tend to stick at a very old version of Go, to the point where I think it's more common to either download/install Go manually or use a third-party PPA. The longsleep/go-backports PPA is the one I've seen used most often, and is updated on a regular cadence. Do we need to get approval for that? @brianjmurrell what do you think?

Latest commits use the external PPA I mentioned here.

Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
@kjacque kjacque requested a review from mjmac September 26, 2024 00:52
Copy link
Contributor

@ashleypittman ashleypittman left a comment

Choose a reason for hiding this comment

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

Without having Ubuntu in the support matrix (which I don't think it is) then Ubuntu dockerfiles are for convenience only and I see no reason why we shouldn't be updating to the latest stable or LTS release. This dockerfile is only invoked from GHA that I'm aware of.

site_scons/site_tools/go_builder.py Outdated Show resolved Hide resolved
site_scons/site_tools/go_builder.py Outdated Show resolved Hide resolved
site_scons/site_tools/go_builder.py Show resolved Hide resolved
site_scons/site_tools/go_builder.py Outdated Show resolved Hide resolved
Required-githooks: true

Signed-off-by: Kris Jacque <kris.jacque@intel.com>
@ashleypittman
Copy link
Contributor

The other thing on the scons check front is that it might be possible to just get the go command to do "something" with the go.mod file and see if fails or not. I'm not sure if there is a suitable go command for this that exists in 1.21 though. It would simplify all the code we have if we could just do a test compile. Out of scope for this PR though.

Comment on lines 17 to 20
# 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.

mjmac
mjmac previously approved these changes Sep 26, 2024
Signed-off-by: Kris Jacque <kris.jacque@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants