Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use modern statements in spec file #326

Merged
merged 2 commits into from
Jan 5, 2023
Merged

Use modern statements in spec file #326

merged 2 commits into from
Jan 5, 2023

Conversation

Olf0
Copy link
Contributor

@Olf0 Olf0 commented Jan 5, 2023

  • %global instead of %define for static expressions: %globals are evaluated once when set, %defines are evaluated each time used. Plus the scope of %globals is all sections, including the scriptlets.
  • -n %{name}-%{version} always has been the default for %setup, hence omitted now.
  • rm -rf %{buildroot} as first statement in the %install section is long obsolete, thus superfluous.

- `%global` instead of `%define` for static expressions: `%global`s are evaluated once when set, `%define`s are evaluated each time used.  Plus the scope of `%global`s is all sections, including the scriptlets.
- `-n %{name}-%{version}` always has been the default for `%setup`, hence omitted now.
-  `rm -rf %{buildroot}` as first statement in the `%install` section is long obsolete, thus superfluous.
@Olf0 Olf0 added the enhancement this improves something label Jan 5, 2023
@Olf0 Olf0 self-assigned this Jan 5, 2023
@Olf0 Olf0 requested a review from nephros January 5, 2023 00:59
@nephros
Copy link
Contributor

nephros commented Jan 5, 2023

I believe the %setup -n foo is required for OBS to work but have to check.

@nephros
Copy link
Contributor

nephros commented Jan 5, 2023

When you say the rm on the bildroot is obsolete, does this mean it is done implicitly?

I know the %clean macro is obsolete, but cleaning the buildroot is useful when building locally.

@nephros
Copy link
Contributor

nephros commented Jan 5, 2023

believe the %setup -n foo is required for OBS to work but have to check.

Scratch that, works fine without it.

@Olf0
Copy link
Contributor Author

Olf0 commented Jan 5, 2023

I believe the %setup -n foo is required for OBS to work but have to check.

Definitely not by OBS in general, but maybe by Jolla's tar_git: I once analysed it somewhere, it is a shell script. Will try to find my analysis.

Edit:
Found my analysis of Jolla's tar_git, AFAICS / IIRC it uses %{name} and extracts %{version} from a git tag, if one is set / used (plus %{release}, but that is heavily processed after its extraction). If no git tag is set or specified (in the OBS' .service file), then tar_git uses the %{version} from the spec file and appends the a part of the hash of the latest commit (in the branch which is configured in the .service file or which tar_git decides to use) to the processed %{release} string.

Actually, it is this behaviour which creates the necessity to have the %{version} in the spec file and in a correspondingly created git tag set to the same version string. But I cannot see, why %{name}-%{version} would be altered by anything so it is not the original string (at any time, including the point in time the %setup macro is called in the %prep section of a spec file).

When you say the rm on the buildroot is obsolete, does this mean it is done implicitly?

I know the %clean macro is obsolete, but cleaning the buildroot is useful when building locally.

Yes, both the %clean macro and rm -rf %{buildroot} are long obsolete, according to the RPM documentation etc. (Fedora packaging guidelines and many more) and I checked that in many rpmbuild runs.

And it is successfully build by our CI workflow, which utilises Coderus' Sailfish-SDK images, hence mb2 and all the other pieces of Jolla's SDK. But you are absolutely right, this way I never checked what happens if the SDK environment has been used before (as these images are always freshly instanciated in a docker container).

Copy link
Contributor

@nephros nephros left a comment

Choose a reason for hiding this comment

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

all good.

@Olf0
Copy link
Contributor Author

Olf0 commented Jan 5, 2023

all good.

I am still glad (every time), that you double-checked and triggered me to re-evaluate and properly document my reasoning.

@Olf0 Olf0 merged commit 0ec1254 into master Jan 5, 2023
@Olf0 Olf0 deleted the Olf0-patch-1 branch January 5, 2023 14:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement this improves something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants