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-17253 build: Fix client only build #16059

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

DAOS-17253 build: Fix client only build #16059

wants to merge 3 commits into from
+116 −180

Conversation

jolivier23
Copy link
Contributor

@jolivier23 jolivier23 commented Mar 8, 2025 •
edited
Loading

  • When tests are specified, some server components are built.
  • Argobots is still built for some mocks but this is acceptable
    for now.
  • Add conditional build to rpm spec
  • Remove server side build from Ubuntu packaging

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

@jolivier23 jolivier23 requested review from a team as code owners March 8, 2025 01:50
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2025

Ticket title is 'client only build is broken'
Status is 'Open'
https://daosio.atlassian.net/browse/DAOS-17253

@jolivier23 jolivier23 force-pushed the jvolivie/client_only branch 4 times, most recently from 603136a to 42aa4e2 Compare March 8, 2025 05:38
@daosbuild1
Copy link
Collaborator

Test stage Build on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16059/4/execution/node/375/log

@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16059/4/execution/node/376/log

daltonbohning
daltonbohning previously approved these changes Mar 10, 2025
View reviewed changes
@jolivier23 jolivier23 force-pushed the jvolivie/client_only branch from 42aa4e2 to 80bb8bd Compare March 10, 2025 19:40
@jolivier23 jolivier23 requested a review from a team as a code owner March 10, 2025 19:40
@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16059/6/execution/node/345/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16059/6/execution/node/339/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16059/6/execution/node/344/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16059/6/execution/node/336/log

@jolivier23 jolivier23 marked this pull request as draft March 10, 2025 20:47
@jolivier23 jolivier23 force-pushed the jvolivie/client_only branch 3 times, most recently from e51ded0 to 5d92824 Compare March 10, 2025 23:47
@jolivier23
DAOS-17253 build: Fix client only build
972141c 
* When tests are specified, some server components are built.
Argobots is still built for some mocks but this is acceptable
for now.
* Add conditional build to rpm spec
* Remove server side build from Ubuntu packaging

Signed-off-by: Jeff Olivier <jeffolivier@google.com>
@jolivier23 jolivier23 force-pushed the jvolivie/client_only branch from 5d92824 to 972141c Compare March 10, 2025 23:48
@jolivier23 jolivier23 marked this pull request as ready for review March 11, 2025 00:02
johannlombardi
johannlombardi previously approved these changes Mar 11, 2025
View reviewed changes
daltonbohning
daltonbohning previously approved these changes Mar 11, 2025
View reviewed changes
debian/changelog
@@ -1,3 +1,10 @@
daos (2.7.101-6) unstable; urgency=medium
[ Jeff Olivier ]
* Remove server build from Ubuntu packaging
Copy link
Contributor

Choose a reason for hiding this comment

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

So entirely removing the server from Ubuntu packaging? Does nobody use that and no plans to?

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 think it's a reasonable assumption. They can use docker container like we do. Anyway, the debian build is a pain to maintain for the server components and we want to update spdk

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to bring that up at a TSC before claiming that.
There were some folks who build from source on ubuntu even if not officially supported.

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, we build from source too. This doesn't remove the ability to build it, just removes the packaging which is difficult to maintain

Copy link
Contributor

Choose a reason for hiding this comment

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

yea at some point we did have a roadmap bullet for 2.8 for supporting ubuntu packaging but it was removed due to resourcing. not sure though if we really want to kill at this point, which is why i suggest to bring this up at a tsc before landing this.

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 know we have wanted that but just for client.

Copy link
Contributor

@grom72 grom72 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As I recall, one of the important factors in keeping Ubuntu was Intel's expectation. I'm not sure that's still the case.
On the other hand, if we provide Ubuntu packages but do not test them at all, how can we know what they represent?
And yes, it will be a huge improvement from a maintenance point of view.
But it should be done separately, not as a part of SPDK upgrade PR where we completely remove Ubuntu support and validation.

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 don't see the point in separation to be honest. The upgrade is the reason I'm doing this.

Anyway this patch needs to land first

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'm not removing all Ubuntu packages, just the server ones.

@jolivier23 jolivier23 changed the base branch from master to release/2.6 March 11, 2025 18:08
@jolivier23 jolivier23 requested review from a team as code owners March 11, 2025 18:08
@jolivier23 jolivier23 changed the base branch from release/2.6 to master March 11, 2025 18:09
mjmac
mjmac requested changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@mjmac mjmac left a comment

Choose a reason for hiding this comment

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

Generally looks fine to me, but the firmware management stuff needs to work for both client and server builds, when it's enabled.

utils/rpms/daos.spec
%bcond_without server

%if %{with server}
%global daos_build_args FIRMWARE_MGMT=yes
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 not a server-only feature. If it's enabled, it needs to be enabled on both client and server sides. I wouldn't be opposed to adding a new bcond_without for it, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there server components needed for the client side? I think we removed it because it failed to build

@jolivier23 jolivier23 removed request for a team March 11, 2025 18:56
mjmac
mjmac approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@mjmac mjmac left a comment

Choose a reason for hiding this comment

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

Removing my -1 wrt firmware management -- needs to be reworked separately.

@jolivier23
Copy link
Contributor Author

Sorry for the re-reviews, I accidently reset the base branch to release/2.6 and back to master branch.

@jolivier23 jolivier23 requested a review from phender March 12, 2025 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grom72 grom72 grom72 left review comments

@mchaarawi mchaarawi mchaarawi left review comments

@mjmac mjmac mjmac approved these changes

@tanabarr tanabarr tanabarr approved these changes

@johannlombardi johannlombardi Awaiting requested review from johannlombardi

@daltonbohning daltonbohning Awaiting requested review from daltonbohning

@phender phender Awaiting requested review from phender

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jolivier23 @daosbuild1 @mjmac @johannlombardi @grom72 @tanabarr @mchaarawi @daltonbohning