Skip to content

Conversation

@webbnh
Copy link
Member

@webbnh webbnh commented Mar 30, 2021

Previous to this PR, the agent and server each had separate makefiles which included separate subordinate rpm.mk files, but overall, the contents were nearly identical. This PR creates a common rpm.mk file, and strips the agent and server makefiles down to little more than an include directive.

I tested by comparing the output of the RPM builds without these changes and to the output with these changes; the differences were all reasonable and expected. And, I was able to install the resulting RPMs in the pbench-devel container.

Some of the items to pay particular attention to:

  • The rpm-dirs target is now a dependency of all the targets which require one or more of the directories which it creates. This means that this target no longer needs to be built explicitly, and it means that the other targets no longer need to explicitly create those directories.
  • The patches target now references the current directory as '.' rather than via a calculated absolute reference. This is simpler and hopefully safe.
  • Previously, the server build modified pbench-config and pbench-server to change the script interpreter to /usr/bin/env python3; this seems to be unnecessary, so I've removed it.
  • The code now uses := instead of = for all variables which are set to expressions which reference values outside the makefile, such as external files or shell call-outs, so that the expressions are evaluated only once.
  • The build now updates the Git submodules unconditionally and recursively. The recursion isn't required for the agent, and the step isn't required at all for the server, but it shouldn't hurt to do so, and it will help "future-proof" the build. Also, the command is issued from the current directory instead of from the source root, but my understanding is that it should work properly regardless of where it is issued from, so long as the current directory is in a Git checkout.
  • I don't think the clean-sha1 target worked before this change; so I removed it.
  • I replaced the veryclean target with distclean and made it clean more than clean does.

Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Minor comments and questions...

@portante portante added this to the v0.71 milestone Mar 30, 2021
@webbnh webbnh marked this pull request as ready for review March 30, 2021 22:11
@webbnh webbnh requested review from dbutenhof and ndokos March 30, 2021 22:11
portante
portante previously approved these changes Mar 30, 2021
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Approving, but if you feel like making a small change, that would be great.

ndokos
ndokos previously approved these changes Mar 31, 2021
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

LGTM - modulo Peter's comment on the cp.

@webbnh webbnh dismissed stale reviews from ndokos and portante via 6d6a4f3 March 31, 2021 17:42
ndokos
ndokos previously approved these changes Mar 31, 2021
@ndokos ndokos merged commit 8c3a102 into distributed-system-analysis:main Mar 31, 2021
@webbnh webbnh deleted the webb/rpm-fun branch March 31, 2021 18:09
portante pushed a commit to portante/pbench that referenced this pull request Apr 13, 2021
* Unify agent and server container makefiles

* Code review feedback

* More code review feedback
riya-17 pushed a commit to riya-17/pbench that referenced this pull request Jul 6, 2021
* Unify agent and server container makefiles

* Code review feedback

* More code review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants