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

debian: introducing nodocs and nocheck DEB_BUILD_OPTIONS (smoe:debian_nocheck) #2647

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

smoe
Copy link
Contributor

@smoe smoe commented Sep 20, 2023

There are environment variables that may affect the Debian packaging - skipping the building of documentation or performing tests is one common use case. The nocheck option may also be given as an argument to the dependencies. So when xvfb is only required for testing purposes, this may overcome a hurdle on the one or other RPi installation. This PR is a reaction to the experience by Gene on the -dev mailing list from mid 9/2023.

@rodw-au
Copy link
Contributor

rodw-au commented Sep 20, 2023

Can you explain what this will achieve? I have built a lot of ISOs and Pi images recently so trying to understand.
How is this different to ./configure no-docs?
What I have noticed is that a lot of the documents are of low quality (eg Lots of English in the Chinese docs etc).
I waste many hours waiting for these to build. I think inclusion of docs should be able to be set per language. I only ever refer to the online docs myself so am comforatable with no-docs.

@smoe
Copy link
Contributor Author

smoe commented Sep 20, 2023

Can you explain what this will achieve? I have built a lot of ISOs and Pi images recently so trying to understand.
Much appreciated!

How is this different to ./configure no-docs?
The Debian packaging is controlled by the debian/rules Makefile. I am not sure about what configure that is - debian/configure? Well, that debian/configure is a very LinuxCNC-specific solution to introduce flexibility. I had not fully thought that through, I must admit, my main concern was to get the xfvb build-dependency out of the way when it is not needed.

What I have noticed is that a lot of the documents are of low quality (eg Lots of English in the Chinese docs etc). I waste many hours waiting for these to build. I think inclusion of docs should be able to be set per language. I only ever refer to the online docs myself so am comforatable with no-docs.

Same here. The Debian-typical way to have no documentation built and no checks done is to

DEB_BUILD_OPTIONS=nodocs,nocheck dpkg-buildpackage

@rodw-au
Copy link
Contributor

rodw-au commented Sep 20, 2023

I think the ./configure is easier for scripting in an image builder
if [[ "$BUILD_DOCS" == "1" ]]; then # We will build the docs
./configure
else
./configure no-docs # Skip building docs
fi

Ref my RPI image builder: https://github.com/rodw-au/rpi-img-builder-lcnc/blob/linuxcnc/files/userscripts/uscripts#L63C1-L68C3

(Which is presently churning away building docs for the just released 6.1.54 kernel update)

I hope to see this forked into the Linuxcnc repos in time for the release of 2.9

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Sep 20, 2023 via email

@rodw-au
Copy link
Contributor

rodw-au commented Sep 20, 2023

Now I am starting to understand.
Could you please ensure that this commit extends to updates to the user docs here
http://linuxcnc.org/docs/2.9/html/code/building-linuxcnc.html#_building_debian_packages which is generated from this file https://github.com/LinuxCNC/linuxcnc/blob/2.9/docs/src/code/building-linuxcnc.adoc

Users must know how to set these parameters and what is the behaviour is.

@smoe
Copy link
Contributor Author

smoe commented Sep 21, 2023

I have no particular agenda, just think that the strict dependency on xfvb should go away since that (to my understanding) was only added for testing purposes, so Gene (and others) can have an easier experience when building the package.
A good Debian package has this kind of context-annotation of its dependencies, so there is little doubt in my mind that our package should have this annotation, too. If the package build instructions are generated by debian/configure or not should not be of concern.

@petterreinholdtsen , I have only a partial recollection of your earlier patch. There was another one being accepted in the meantime if I recall correctly. Will have another look at some point. This one was meant to be a quick one to help Gene out, I should not have added those nodocs one, though.

@rodw-au , I think the difference is that with debian/configure you can create a variant of package descriptions that just do not know anything about documentation packages to exist - debian/control then becomes just very short. The typical Debian-way would be to alway have a fully-complete debian/control, you just decide not to build the documentation packages. This then reduces compile times and reduces the dependencies needed for the build that need to be installed but leaves the debian folder with all the instructions unchanged, so you can use what is shipped with the Debian distribution and still have a reduced complexity.

@smoe
Copy link
Contributor Author

smoe commented Sep 21, 2023

Now I am starting to understand. Could you please ensure that this commit extends to updates to the user docs here http://linuxcnc.org/docs/2.9/html/code/building-linuxcnc.html#_building_debian_packages which is generated from this file https://github.com/LinuxCNC/linuxcnc/blob/2.9/docs/src/code/building-linuxcnc.adoc

Users must know how to set these parameters and what is the behaviour is.

Yes, I happily add this info and respective pointers to the Debian documentation on these to our documentation.

@rodw-au
Copy link
Contributor

rodw-au commented Sep 21, 2023

Steffen, thanks for the clarification. it sounds the way to go. Sometimes I have found depedencies that are not reported by dpkg-checkbuilddeps so the control file has not been updated.. With my image builders, its only the Rapberry Pi one this impacts on because the new gpio driver is only in master branch so I build from source. The driver needs to be ported back to 2.9 or this needs to be ported to master before it would affect the builder. There are no debs built for the pi so it has to be built from source.
The X64 builder can grab the latest debs from the new buildbot.
I'm a bit like you, I had no agenda either except I figured we would need the builders before 2.9 could be released so I thought I could contribute something.

@smoe
Copy link
Contributor Author

smoe commented Sep 21, 2023

I liked your build script.
Have now added to the build instructions what came to my mind. Did not yet have the opportunity to actually test those flags, which feels a bit bad.

@rodw-au
Copy link
Contributor

rodw-au commented Sep 21, 2023

Have now added to the build instructions what came to my mind. Did not yet have the opportunity to actually test those flags, which feels a bit bad.

Is this correct? The debian/configure script takes a single argument

It would be helpful to add a note when discussing satisfying build dependencies that copying the list into a text editor to remove version information and alternatives, then paste the list back into the terminal saves a lot of time.

It is an awesome, badly needed documentation makeover Steffen!

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Sep 21, 2023 via email

@rodw-au
Copy link
Contributor

rodw-au commented Sep 21, 2023

Are you talking about something besides 'apt build-dep .' here? No need to copy and edit build dependencies.

Actually here https://github.com/smoe/linuxcnc/blob/debian_nocheck/docs/src/code/building-linuxcnc.adoc?plain=1#L331

You woud still need to do this if you are doing a run in place build

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Sep 22, 2023 via email

@rodw-au
Copy link
Contributor

rodw-au commented Sep 22, 2023

There are a few issues like that which need cleaning up. I was working with an experienced friend today and he had some issues so I suggested he upgrade from the Debian 12 linuxcnc-uspace to the latest 2.9 deb at our new buidbot with apt pinning. Linuxcnc was broken on startup due to issue #2510 that was fixed in master (2.10) via commit fced097. That commit needs porting back to 2.9 urgently. So then I told him to update his sources list to get master-uspace from buildbot2.

It failed to install from the repo due to a missing dependency libgpiod2. It must be missing from the control file. But it did eventually fix the broken 2.9 version.

I think libgpiod2 was added to support a new GPIO driver Andy has added to 2.10 for the Raspberry pi. While it does exist in AMD64, I wonder if it should be restricted to ARCH=ARM64?

@smoe
Copy link
Contributor Author

smoe commented Sep 22, 2023

[Rod Webster]
Actually here https://github.com/smoe/linuxcnc/blob/debian_nocheck/docs/src/code/building-linuxcnc.adoc?plain=1#L331 You woud still need to do this if you are doing a run in place build
Right. Look like it should be rewritten to use 'apt build-dep .'.

Which would then remove the section on dpkg-checkbuilddeps? I did not want to change too much (and admittedly use dpkg-buildpackage for everything). Anyone of yours feeling like sending a PR to my branch?

@smoe
Copy link
Contributor Author

smoe commented Sep 22, 2023

libgpiod2

There is https://tracker.debian.org/pkg/libgpiod that has a libgpio2 package but the version of that all is 1.6.3 with https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git showing version 2.0.2 and 1.6.4. So, I think we should update the Debian package to 1.6.4 and also come up with a second package to address the new major version - I just feel surprised that the binary's name is libgpio2 already - that number needs to match the soname of that library, not the package version, that likely is why.
You are right- debian/control.top.in does not list the libgpio2 dependency. Should this be added for all architectures? Or only for the Pi?

@smoe
Copy link
Contributor Author

smoe commented Sep 22, 2023

Is there a quick'n'easy way to help our release manager with a PR to get the backport of the #2510 fix done?

@rodw-au
Copy link
Contributor

rodw-au commented Sep 22, 2023

Steffan, its libgpiod2 https://packages.debian.org/bookworm/libgpiod2
There is probably no harm in having it in x86, there is a pc with gpio ports! Odroid H2+ and H3+ and I have a H2+ So it should stay in.

I think there are some other dependencies missing too as I added some to the pi builder along the way but did not record them. They are probably build dependencies.

Also, a user used my ISO today and it had brltty and speech-dispatcher on it which he found terribly annoying. I don't install them so they must get carried through from Debian with live-build
I will put a rule in my image builder to see if they can be left out.

If you are chasing a release manager, see if Andy can backport his pi gpio hal driver to 2.9 as well. Its been tested on the forum now.

@smoe
Copy link
Contributor Author

smoe commented Sep 22, 2023

The binary package is libgpio2, but the source package producing that library does not have the "2" in its name (https://tracker.debian.org/pkg/libgpiod, binaries listed in the lower-left) and the version of that package in Debian currently is 1.6.3 with a version 2.0.2 available upstream.

I agree that this package should be added as a build-dependency. At the very moment I just do not know if it then also becomes a strict runtime dependency or if it is something that should just be recommended since it can be loaded dynamically when requested by HAL.

@andypugh , please direct us a bit. What kind of PRs would you like to see for these backports / additions to the build-deps?

@gmoccapy
Copy link
Collaborator

gmoccapy commented Oct 7, 2023

@smoe please update this PR

Comment on lines +60 to +66
ifeq (,$(filter nodocs,$(DEB_BUILD_OPTIONS)))
ifneq "$(enable_build_documentation)" ""
dh_auto_build -- manpages
dh_auto_build -- translateddocs
dh_auto_build -- docs
endif
endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really a good idea to have two ways to set the same flag? I suspect this will break the principle of least surprise. What should happen if someone enable documentation building and use the nodocs option? Which one should take effect? I suspect it is better to only have one flag, and allow both the option and the enable setting to set or unset this flag. At the moment this patch allow nodocs to override enable_documentation. Is that least surprising?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to ensure enable_build_documentation is "" if nodocs is set, and use only this test in the code, to ensure not two partly overlapping settings are used in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just had a look at it again. The "enable_build_documentation" is set by debian/configure.in - and I agree with you, this should just always be enabled and the decision to build or not be left to Debian's regular semantics, i.e. the DEB_BUILD_OPTIONS parameters. The point of this patch is to help transitioning towards that change of control. So, now you can decide not to package the documentation (by adding the nodocs) even though the configuration (by debian/configure) was expecting you to build it. The other way around does not work - you cannot enable the building of the documentation by omitting the nodocs if you have previously disabled it in debian/configure. I tend to think this is all very much how it should be - given the constraint to impose minimal changes to the current code base.

On a sidenote, at the beginning of debian/rules.in, there is

ifeq (,$(filter nodocs,$(DEB_BUILD_OPTIONS)))
enable_build_documentation = @ENABLE_BUILD_DOCUMENTATION@
endif

so enable_build_documentation will only be available if nodocs is not an option. I tend to think I am happy with how it is. I am uncertain if we should already use the nodocs/nocheck flags in our CI.

Copy link
Collaborator

@petterreinholdtsen petterreinholdtsen left a comment

Choose a reason for hiding this comment

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

The nocheck part seen to be obviously correct. Not quite convinced about nodocs vs. enable docs.

@smoe smoe marked this pull request as ready for review October 20, 2023 09:15
@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Oct 20, 2023 via email

@smoe
Copy link
Contributor Author

smoe commented Oct 22, 2023

The "enable_build_documentation" is an option to configure. I would prefer to leave it as it is (minimal changes) and then transition towards your #1835, we just need to figure out how to get the Debian packaging adjusted for other real time environments. We could argue that this patch should be against 2.10 not 2.9.

@rodw-au
Copy link
Contributor

rodw-au commented Oct 22, 2023

We could argue that this patch should be against 2.10 not 2.9.

I think this would be a great idea becasue Andy and I have finalised the AMD64 Live image installer and Raspberry Pi Bootable Image over the weekend.

I always thought new features should go in the master branch (currently 2.10)

@smoe smoe changed the base branch from 2.9 to master October 22, 2023 22:10
@smoe
Copy link
Contributor Author

smoe commented Oct 22, 2023

Rebased on master and adjusted PR.

@smoe
Copy link
Contributor Author

smoe commented Oct 22, 2023

I always thought new features should go in the master branch (currently 2.10)

Yes, just that in my reading this was more like a fix of the Debian packaging, not a new feature. But 2.9 is now out and there is no need to backport this to 2.8 either.

@rodw-au
Copy link
Contributor

rodw-au commented Oct 22, 2023

But 2.9 is now out and there is no need to backport this to 2.8 either.

I really wish 2.9 was out and sent to Debian. If there is anything you can do to help Andy make it happen, please contact him, particularly uploading to Debian.

Maybe this can be backported to 2.9.3 when its released...

@andypugh
Copy link
Collaborator

andypugh commented Nov 1, 2023

I really wish 2.9 was out and sent to Debian. If there is anything you can do to help Andy make it happen, please contact him, particularly uploading to Debian.

Steffen uploaded 2.9.1 to Debian a few days ago.
The only roadblock to the full 2.9.1 release is me finding the time to edit the release / install notes and send the notifications.

That and an unwillingness to give up on RTAI packages.

@andypugh
Copy link
Collaborator

andypugh commented Nov 1, 2023

My experience recently has been that if you configure-out docs building then the packaging phase errors out, as there is no conditionality on building the docs packages, and dh_movefiles finds no files to move in the nodocs case.

With this patch, do you actually end up with a working LinuxCNC .deb file at the end?

@smoe
Copy link
Contributor Author

smoe commented Aug 17, 2024

Have just rebased to master again. Am not ultimately happy. Have added a few lines to debian/configure that instruct how these options can hlp but I am not ultimately confident that this was very helpful. What should be avoided though is that new users build the documentation for all the language. That sucks.

The best thing I came now up with was fakeroot ./debian/rules binary-arch. The problem with dpkg-buildpackage is that it expects a properly named tarball, which may not be present.

@rodw-au
Copy link
Contributor

rodw-au commented Aug 17, 2024

I cloned Steffen's repository and can confirm it all works perfectly even for a noob who knows nothing about Debian Packaging!
My only comment would be it might be nice to add a summary as done in the quickstart section eg

For the impatient, try this to build the deb files:

$ cd linuxcnc-dev
$ ./debian/configure
$ sudo apt-get build-dep .
$ DEB_BUILD_OPTIONS=nocheck dpkg-buildpackage -uc -B

Please commit this in the interest of widespread user sanity!

@smoe
Copy link
Contributor Author

smoe commented Aug 17, 2024

I have just integrated @rodw-au 's comment. Should the instructions at the end of debian/configure be changed accordingly?

@rodw-au
Copy link
Contributor

rodw-au commented Aug 17, 2024

I have just integrated @rodw-au 's comment. Should the instructions at the end of debian/configure be changed accordingly?

Sorry I wasn't clearer.
Its not really where I had in mind where this should go as that section is all about compling for a RIP install I was that there was an identical similar section for debs under Building Debian packages. Maybe here.
https://github.com/smoe/linuxcnc/blob/debian_nocheck/docs/src/code/building-linuxcnc.adoc?plain=1#L242

The good thing about your Debian way is that it still allows building for RIP after it is run and it gives a super simple way to get the build dependencies installed which was always a nightmare,

@smoe
Copy link
Contributor Author

smoe commented Aug 21, 2024

May I ask you to prepare for a PR against this branch of mine with what you have in mind? I just somehow guess that what you'd write will be different from what I'd write and your perspective likely is the more important one for the LinuxCNC folks.

@smoe smoe changed the title debian: introducing nodocs and nocheck DEB_BUILD_OPTIONS debian: introducing nodocs and nocheck DEB_BUILD_OPTIONS (smoe:debian_nocheck) Aug 31, 2024
@smoe
Copy link
Contributor Author

smoe commented Aug 31, 2024

Rebased to master and integrated instructions by @rodw-au that arrived by PM. Many thanks!

@rodw-au
Copy link
Contributor

rodw-au commented Sep 9, 2024

Is there any reason stopping this from being merged?

@andypugh
Copy link
Collaborator

Is there any reason stopping this from being merged?

Well, it's failing the checks at the moment.
And I thought it was still draft?

@rodw-au
Copy link
Contributor

rodw-au commented Sep 10, 2024

I don't think it needs to be a draft anymore as I tested it and reviewed/modified the docs but I can't help with the failing tests.

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.

5 participants