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

Support for cross-platform RPM package generation #729

Open
wants to merge 1 commit into
base: 0.9.x
Choose a base branch
from

Conversation

ogalbxela
Copy link

@ogalbxela ogalbxela commented Aug 13, 2023

Please consider a patch aimed to add proper support for constructing RPM packages aimed for the CPU architecture other than the host machine. This addresses #727 issue.

The current version of the "pkg_rpm"-rule exposes an "architecture"-option, which is meant to indicate the package's intended platform. Unfortunately, this option is being incorrectly utilized. It is being used to define the "BuildArch"-option in the RPM spec file. However, as per RPM packaging guide (https://rpm-packaging-guide.github.io/):
"'BuildArch' should be used if the package is not architecture dependent. For example, if written entirely in an interpreted programming language, set this to BuildArch: noarch. If not set, the package automatically inherits the Architecture of the machine on which it is built".

It appears that BuildArch is intended to represent either architecture of host machine or "noarch". Any efforts to designate an "alien" platform as a value for this field lead to a failure in the "rpmbuild"-utility, accompanied by an error message stating: "error: No compatible architectures found for build."

The purpose of this patch is to maintain backward compatibility while provide the solution for the described issue.
It preserves the existing "architecture"-attribute of the "pkg_rpm" rule, as it might already be in use by someone.
It introduces the "target_architecture"-attribute to the "pkg_rpm" rule, aiming to utilize it as a "arch"-suffix for the package name and to provide it to the "rpmbuild"-utility as the value for the "--target" option.

This change introduces proper support for generating RPM packages for the
designated target platform.

The current version of the "pkg_rpm"-rule exposes an "architecture"-option,
which is meant to indicate the package's intended platform.
Unfortunately, this option is being incorrectly utilized. It is being used to
define the "BuildArch"-option in the RPM spec file.
However, as per RPM packaging guide (https://rpm-packaging-guide.github.io/):
  "'BuildArch' should be used if the package is not architecture dependent. For
  example, if written entirely in an interpreted programming language, set this
  to BuildArch: noarch. If not set, the package automatically inherits the
  Architecture of the machine on which it is built".

So, it sounds like it actually has nothing to do with target architecture. It is
supposed to represent either architecture of host machine or "noarch".

The purpose of this patch is to maintain backward compatibility while provide
the solution for the described issue.
It preserves the existing "architecture"-attribute of the "pkg_rpm" rule, as
it might already be in use by someone.
It introduces the "target_architecture"-attribute to the "pkg_rpm" rule, aiming
to utilize it as a "arch"-suffix for the package name and to provide it to the
"rpmbuild"-utility as the value for the "--target" option.
@aiuto
Copy link
Collaborator

aiuto commented Aug 14, 2023

Can we figure the right behavior out in the issue before jumping to code.
I'll comment there.

@ogalbxela
Copy link
Author

Can we figure the right behavior out in the issue before jumping to code. I'll comment there.

Thank you, Tony, I answered in the issue.

@@ -462,7 +467,10 @@ def main(argv):
help='The release of the software being packaged.')
parser.add_argument(
'--arch',
help='The CPU architecture of the software being packaged.')
help='The CPU architecture of the machine on which it is built. '
'If the package is not architecture dependent, set this to "noarch".')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this comment apply to targert_arch?

@@ -791,21 +794,30 @@ pkg_rpm = rule(
# funny if it's not provided. The contents of the RPM are believed to
# be set as expected, though.
"architecture": attr.string(
doc = """Package architecture.
doc = """Host architecture.

This currently sets the `BuildArch` tag, which influences the output
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's drop "currently" in these

@@ -791,21 +794,30 @@ pkg_rpm = rule(
# funny if it's not provided. The contents of the RPM are believed to
# be set as expected, though.
"architecture": attr.string(
doc = """Package architecture.
doc = """Host architecture.

This currently sets the `BuildArch` tag, which influences the output
architecture of the package.

Typically, `BuildArch` only needs to be set when the package is
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might as well make the exact behavior and use clear and tie it back to Bazel definitions when the terms overlap. The points to make are

  • If the package is really architecture independent, say "noarch"
  • Otherwise specify a CPU architecture of the target platform your are building for.
  • If you say nothing, it will default to the CPU arch of your execution platform.

Note that when cross compiling the target and execution platforms may not match.

Choose a reason for hiding this comment

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

@aiuto @ogalbxela when cross-compiling, target/exec may differ, but the logical default value would be the target.

When not cross-compiling, the target and exec match, so the logical default value would be the target.

Is there interest in setting the default to the target rather than exec platform? Or a risk of breaking change in the default ?


When no attribute is provided, this will default to your host's
architecture. This is usually what you want.

""",
),
"target_architecture": attr.string(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need both? I have not figured out the value of setting both, since only one gets used.

@aiuto aiuto added the P2 An issue that should be worked on when time is available label Nov 6, 2023
@aiuto
Copy link
Collaborator

aiuto commented Nov 6, 2023

Friendly ping

@jiceatscion
Copy link

Any plan to ever merge this? It is pity that this very convenient rule cannot be used for general rpm builds, while a solution is just sitting here in limbo.

@aiuto
Copy link
Collaborator

aiuto commented Oct 10, 2024

Any plan to ever merge this? ...

Good question. I keep seeing it in the overview as waiting on the original author, so I have been ignoring it.
It does seem OK though. I can look over the weekend.

@jiceatscion
Copy link

Any hope off seeing this merged?

@aiuto
Copy link
Collaborator

aiuto commented Oct 31, 2024

Any hope off seeing this merged?

Yes. If you can work with me on this. Have you patched it in and tried it? Does it have the right behaviors?
The original author went cold on answering my questions.

@jiceatscion
Copy link

jiceatscion commented Oct 31, 2024

Any hope off seeing this merged?

Yes. If you can work with me on this. Have you patched it in and tried it? Does it have the right behaviors? The original author went cold on answering my questions.

Awsome, I will gladly try this and get back to you!
...
[10 mn later]
Works like a charm. This made me an arm64pkg from cross-compiled srcs on a x86_64 host. I did not clone the repo to test the tests, just added the patch (minus tests) to my workspace. I presume you've got all the CI you need, but if it helps, I can try and run the tests for you; let me know.

Testing update: I ran into an issue on our CI build, which runs on a different Linux distribution than what I tested on myself. When given (correctly) a target_platform argument, rpmbuild fails; complaining about some undefined macro (_smp_build_ncpus). I don't think it is in any way caused by this change to pkg_rpm. It is a bug in rpmbuild (rpm-software-management/rpm#2265) that is made accessible by the ability to invoke buildrpm for a cross-platform packaging). I am not sure that pkg_rpm should work around it but since /I/ have to: adding the argument : defines = {"_smp_build_ncpus" : "1" }, fixes the problem.

jiceatscion added a commit to jiceatscion/scion that referenced this pull request Oct 31, 2024
This is enabled by a pending PR to the rules_pkg project:
bazelbuild/rules_pkg#729

I just added the patch to out build while we wait for the PR to get merged.
jiceatscion added a commit to scionproto/scion that referenced this pull request Nov 5, 2024
This is enabled by a pending PR to the rules_pkg project:
bazelbuild/rules_pkg#729

I just added the patch to our build while we wait for the PR to get
merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 An issue that should be worked on when time is available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants