-
Notifications
You must be signed in to change notification settings - Fork 70
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
base: master
Are you sure you want to change the base?
Conversation
xml/art-obs-beginners-guide.xml
Outdated
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>. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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 :/
9e7e623
to
25e23c5
Compare
Here is my second try. Could you please review it? |
There was a problem hiding this 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. 🙂
</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> |
There was a problem hiding this comment.
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:
</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> |
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> |
There was a problem hiding this comment.
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.)
No description provided.