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

feat: add option to set %_target_os #175

Merged
merged 3 commits into from
May 5, 2021

Conversation

hicom150
Copy link
Contributor

@hicom150 hicom150 commented Aug 3, 2020

This PR adds the option to define the %_target_os that it is used in the target option of rpmbuild.

--target PLATFORM
When building the package, interpret PLATFORM as arch-vendor-os and set the macros %_target, %_target_cpu, and %_target_os accordingly.

This PR is related to webtorrent/webtorrent-desktop#1849 as the maintainer uses osx (darwin) to generate linux packages.

@malept malept changed the title Add option to set %_target_os feat:add option to set %_target_os Aug 3, 2020
Copy link
Collaborator

@fcastilloec fcastilloec left a comment

Choose a reason for hiding this comment

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

I'm sorry but RPM will always override all %_target, %_target_arch, and %_target_os no matter what values we decide to put here.
If these values will be allowed to be set by the user, you could simply just use .rpmmacros to override them, or just make changes to the installer so it can accept arch: x86_64-felipe-gnu rather than two additional options.
This is all done so people don't build for a wrong architecture and OS, you can read more here https://docs.fedoraproject.org/ro/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch-rpmbuild.html

test/installer.js Outdated Show resolved Hide resolved
@hicom150
Copy link
Contributor Author

hicom150 commented Aug 4, 2020

Thank you @fcastilloec for your feedback! 👍

I think I've not explain myself very well 😅 My intention was to change %_target_os but throw rpmbuild --target option composing with ${this.options.arch}-${this.options.vendor}-${this.options.os} so we can create rpm package targeted to a different OS than the host OS.

This is based on the rpmbuild documentation where it says

--target PLATFORM
When building the package, interpret PLATFORM as arch-vendor-os and set the macros %_target, %_target_cpu, and %_target_os accordingly.

In my case, with this PR merged, I'm able to successfully create a linux targeted rpm from a macOS (darwin) host.

@hicom150
Copy link
Contributor Author

Given that #176 has been already merged, do you find possible to try to push this forward? 😉

@hicom150 hicom150 requested a review from malept August 28, 2020 08:58
@feross
Copy link

feross commented Nov 9, 2020

@fcastilloec is it possible to address @hicom150's response to your feedback? ❤️

Base automatically changed from master to main February 6, 2021 18:41
@fcastilloec
Copy link
Collaborator

I'll be taking another look at this PR in the next weeks. I've just been very busy.

I'm still a little hesitant into introducing this feature, I do believe that it's always better to build the package in the same machine that's intended for use. Nonetheless, I see that this is not always possible, even with so many free CI services around.

Copy link
Collaborator

@fcastilloec fcastilloec left a comment

Choose a reason for hiding this comment

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

I've made some changes. The most important ones: rebasing against our latest main branch so we can run the tests under macOS, and fixing the tests which weren't asserting anything.

@fcastilloec fcastilloec changed the title feat:add option to set %_target_os feat: add option to set %_target_os May 5, 2021
@fcastilloec fcastilloec merged commit e8d13d5 into electron-userland:main May 5, 2021
README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants