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

Review of the Build Recipe section #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

libnoon
Copy link
Contributor

@libnoon libnoon commented May 18, 2021

No description provided.

The build commands generally requires that some other packages
are installed beforehand; for example, to build a C++ program,
you need a C++ compiler. You need to indicate this list of
packages: the <emphasis>build requirements</emphasis>.
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is way to broad for our documentation here. We do not want to document package formats in general here, just concepts and OBS specifics.

Copy link

Choose a reason for hiding this comment

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

I think @tomschr wrote this from the perspective of OBS allowing to create many types of packages/images, but RPM still being the most important of those. As such, it does make sense to me. I also would not overburden the Beginner's Guide with too many theoretical concerns that do not affect RPM.

(I have to say that on the whole, this PR does not make much sense to me in its current form -- the wording updates it makes generally seem to make it less specific/useful.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for your review, it is much appreciated.
On my side, I do deb packages and I'm interested in OBS to perform this task, so I'm actually trying to learn from its documentation; however certain things do not map correctly for deb packages and that's the main reason I'm proposing changes. I'm trying to make it more universally correct while not complicating the reading. My goal is that the next person in my case will have a less hard time.
I'm happy to take any advice to make it better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would make sense to create two Beginners Guides: one for RPM base (openSUSE) and one for DEB (Debian)?
It also depends probably on how much text we could share between the two.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make sense to create two Beginners Guides: one for RPM base (openSUSE) and one for DEB (Debian)?
It also depends probably on how much text we could share between the two.

Packaging

For openSUSE based RPMs, we usually push people to read the english openSUSE wiki. This is our reference documentation. Especially as it allows anyone to quickly adjust the content, once new features (including package macros) end up in the openSUSE distribution. With this, I would recommend to exclude the openSUSE packaging part completely and instead refer to the wiki.

About Debian / Fedora - or any other packaging: IMHO we should only refer to the specific pages in the openSUSE wiki. Things are really changing quickly - and people usually tend to not send PRs to Github, but edit the wiki instead.

Administration

A big difference are IMHO the configuration settings for the different distributions.

These settings have to be done by the administrators of the Build Service. Here the wiki currently lacks this documentation, which is no surprise, as OBS usually/magically (thanks to Adrian) provides them. But this is also what makes the live of any OBS admin hard: he has to figure out "how Adrian did this"? :-)

<para>
Installation requirements are dependencies which are needed when installing
the final package.
</para>
Copy link
Member

Choose a reason for hiding this comment

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

oh, it was there already :/

@libnoon
Copy link
Contributor Author

libnoon commented Sep 2, 2021

Here is my second try. Could you please review it?
It seems one check failed, but I don't understand how my change could possibly cause this failure?

Copy link
Contributor

@tomschr tomschr left a comment

Choose a reason for hiding this comment

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

@libnoon I found your validation problems. See below. 🙂

Comment on lines +78 to 85
</para>
<para>
These commands often require other packages to be installed
beforehand; for example, to build a C++ program, you need a
C++ compiler. You need to indicate this list of packages: the
<emphasis>build requirements</emphasis>.
</para>
</formalpara>
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 the reason for your validation problem. The <formalpara> element allows only one <para> element, not two (as in your case).

It's quite easy to fix. Just move the end tag </formalpara> a bit higher:

Suggested change
</para>
<para>
These commands often require other packages to be installed
beforehand; for example, to build a C++ program, you need a
C++ compiler. You need to indicate this list of packages: the
<emphasis>build requirements</emphasis>.
</para>
</formalpara>
</para>
</formalpara>
<para>
These commands often require other packages to be installed
beforehand; for example, to build a C++ program, you need a
C++ compiler. You need to indicate this list of packages: the
<emphasis>build requirements</emphasis>.
</para>

Comment on lines +92 to 99
package.
</para>
<para>
To successfully install and remove a package and all its contents,
the package manager needs to know which files and directories belong to
which package.
The installed package may require other packages to function
properly. They are called the <emphasis>installation
requirements</emphasis>.
</para>
</formalpara>
Copy link
Contributor

@tomschr tomschr Sep 3, 2021

Choose a reason for hiding this comment

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

Same here. You need to move your end tag </formalpara> from line 99 after the </para> from line 93.
(GitHub doesn't allow me to add suggestions over deleted lines.)

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.

4 participants