Issues with both building/install methods

[ArchangeGabriel]

Until now I used the traditional setuptools building way:

python setup.py build
python setup.py install --root="${pkgdir}" --skip-build --optimize=1

But with 3.9.4 it fails in the second step with:

error: install_script 'util/DisplayCAL_postinstall.py' not found in scripts

So I’ve tried to switch to the new way using build/installer:

python -m build --wheel --no-isolation
python -m installer --destdir="${pkgdir}" dist/*.whl

But then some files are misplaced:

usr/lib/python3.10/site-packages/build/.config/autostart/z-displaycal-apply-profiles.desktop
usr/lib/python3.10/site-packages/etc/udev/rules.d/45-Argyll.rules

instead of

etc/xdg/autostart/z-displaycal-apply-profiles.desktop
usr/lib/udev/rules.d/45-Argyll.rules

Also, note that these udev rules are already provided by ArgyllCMS, so they should not be installed by DisplayCAL I think.

[eoyilmaz]

Can you try the latest of the develop branch, I just fixed a couple of issues around those files.

[ArchangeGabriel]

Any specific commit in mind that I should backport? As stated in #103 I cannot build from direct source apparently, so I will have to add them on top of latest release: the less they are, the better.

[eoyilmaz]

Are these instructions not working for you https://github.com/eoyilmaz/displaycal-py3/tree/develop#how-to-install

[eoyilmaz]

If not, I can provide a source tar.gz file for you.

[ArchangeGabriel]

No, #103 is what I get when I run python -m build --wheel --no-isolation. Also I cannot run your exact instructions since I’m building for Arch Linux and we have packaging guidelines (so no pip for instance). But it should not matter that much.

But yeah, a tar.gz would be nice indeed.

[eoyilmaz]

Here you are: DisplayCAL-3.9.5.tar.gz
freshly cooked for your taste!

[ArchangeGabriel]

OK, no change regarding this issue, files are still installed in the wrong place:

etc/							      <
etc/xdg/						      <
etc/xdg/autostart/					      <
etc/xdg/autostart/z-displaycal-apply-profiles.desktop	      <
							      >	usr/lib/python3.10/site-packages/build/
							      >	usr/lib/python3.10/site-packages/build/.config/
							      >	usr/lib/python3.10/site-packages/build/.config/autostart/
							      >	usr/lib/python3.10/site-packages/build/.config/autostart/z-displaycal-apply-profiles.desktop
							      >	usr/lib/python3.10/site-packages/etc/
							      >	usr/lib/python3.10/site-packages/etc/udev/
							      >	usr/lib/python3.10/site-packages/etc/udev/rules.d/
							      >	usr/lib/python3.10/site-packages/etc/udev/rules.d/45-Argyll.rules

[eoyilmaz]

I think you shouldn't specify the --destdir="${pkgdir}" option.

Also, what is the output of the following command:

echo $XDG_DATA_DIRS

[ArchangeGabriel]

The destdir arg is mandatory for our build system, it tells the installer to not install into the real root but a fake one that will be used to tar the content of the package.

XDG_DATA_DIRS is not defined in the build environment either.

[eoyilmaz]

Okay so I over looked the #103 and provided a tarball and closed it.

But let me try to understand what is going on here. The GitHub provided tarball was not working for your, that's normal, the tarball that I was providing should be working fine. And I still don't understand why you need the --destdir. You should not use your system python. You should create a virtualenv and use the provided python executable.

My instructions should allow you to install DisplayCAL to a virtualenv. So, isn't that possible in Arch? I'm not familiar with Arch sorry.

[ArchangeGabriel]

I’m not trying to “install” DisplayCAL, I’m packaging it for Arch Linux. This requires to use system python (no venv), system libs (so no pip install of requirements), and to put the DisplayCAL install at a given place using --destdir so that our packaging system can tar the content and make it a package. You can explore our package content at https://archlinux.org/packages/community/x86_64/displaycal/download.

Until 3.9.3 included a setuptools install would work perfectly well, putting each file at its correct place. However such a build is deprecated (stated for removal with python 3.12), and a build/wheel install almost work but has the two configuration files mentioned above installed in a wrong place.

[eoyilmaz]

Aaah now I understand. I don't think we are ready for packaging it up yet. There are some issues around the Open Source license. We have not granted Florian's consent. We might need to change the project's name #96.

[ArchangeGabriel]

Well the pressure to remove python2 is bigger than the interrogations around the licensing on our side (we could just carry the changes as a patch atop latest Florian’s release, that would be the same for us and there would likely be no legal issue with that). So I’ve been packaging your fork since your first announced release.

[eoyilmaz]

Okay, fair enough. So, I think it is time to start working for packaging related issues then... Let's solve this one...

[eoyilmaz]

As I see the desktop file location is coming from this line:

displaycal-py3/DisplayCAL/worker.py

Line 10468 in eb917f5

desktop_file_path = os.path.join(autostart_home, f"{name}.desktop")

and then the autostart_home comes from this line:

displaycal-py3/DisplayCAL/defaultpaths.py

Line 409 in eb917f5

autostart_home = os.path.join(XDG.config_home, "autostart")

and XDG.config_home is defined here:

displaycal-py3/DisplayCAL/defaultpaths.py

Line 193 in eb917f5

config_home = getenvu("XDG_CONFIG_HOME", expandvarsu("$HOME/.config"))

it needs the XDG_CONFIG_HOME otherwise it will use $HOME/.config folder and because you said XDG_DATA_DIRS is not defined at the build environment, thus I believe XDG_CONFIG_HOME is not defined either (Update: I see that it is also using the XDG_CONFIG_DIRS variable along with XDG_CONFIG_HOME), then it uses the default value, and I guess $HOME is resolved to /usr/lib/python3.10/site-packages/build/ and it leads us to the wrong value of: usr/lib/python3.10/site-packages/build/.config/autostart/z-displaycal-apply-profiles.desktop.

I'm curios how it was working with the setuptools way previously.

Can you try defining XDG_CONFIG_DIRS as /etc/xdg before building the package. This would be meaningless...

Update:

So, I see that we need the XDG_CONFIG_DIRS to be present to install the .desktop file. I don't understand while it is not working. Am I getting this correctly: You package DisplayCAL without any problems, but then when you install DisplayCAL using the package it puts those files in to the incorrect places?

By the way, as I see the .desktop file is only installed when you install a profile. So it has nothing to do with the packaging, right? May be you are interpreting it incorrectly, may be it is a left over file from a previous profiling?

Tbh, I have no idea right now, I need a little guidance from your side.

[ArchangeGabriel]

Installing is not involved at any point here, for what is implied here installing is just extracting the package tar on the system. So if things are correct at build time, they will be correct at install time (installation is done by the Arch package manager, pacman).

The paths I’ve listed above are relatives to the $pkgdir folder, which tells you where they will end up upon installation on the system.

A setuptools build put the udev and autostart files at the standard places, a build/wheel one does not. Using setuptools the desktop file is at the autostart path right away, even if they are no profile yet. It might not be necessary indeed, and installation together with a profile would make sense, but I don’t know whether that works.

I don’t know how setuptools get the correct path, python packaging is something I’m only a user of. I’ll ask our python packaging expert.

[FFY00]

The reason it used to work is because setup.py install used the long deprecated egg format, and setuptools could install files to arbitrary locations on the filesystem. This is not supported by wheels.

This is done by the data_files option, here

displaycal-py3/DisplayCAL/setup.py

Line 980 in 3b25f05

"data_files": data_files,

Which is deprecated.

https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-data-files

I recommend reading this, which explains how to properly migrate to wheels.

https://setuptools.pypa.io/en/latest/userguide/datafiles.html

If you want some reasoning why this happened, you can check this out.

pypa/setuptools#2648

[eoyilmaz]

Okay thank you for the links, I'll read them.

[eoyilmaz]

So, I've somewhat looked in to the suggested documentations.

As I see, one of the solutions to remove the data_files that the documentation is suggesting to is to include the required data files in the manifest.in file. And I see that is exactly what Florian was doing at this line:

displaycal-py3/DisplayCAL/setup.py

Line 1527 in 3b25f05

for _datadir, datafiles in attrs.get("data_files", []):

But, I see that the include_package_data attribute is only set for darwin or windows:

displaycal-py3/DisplayCAL/setup.py

Line 1023 in 3b25f05

attrs["include_package_data"] = (

So, I've updated those lines ( 41a6b11 ) to also work with Linux, let's see if it is going to fix your problem.

[eoyilmaz]

Ah you need a tarbal, right? DisplayCAL-3.9.5.tar.gz

[ArchangeGabriel]

This seems worse and installed even more files into the Python location. But actually what @FFY00 tried to say is that you cannot handle it anymore under the modern way, that’s it. So these files should be handled manually by packagers on Linux (dunno how that works with pip though, I’ve asked…), just like I already do: for now I’m moving them from the location they get embedded at with the wheel, but if they are not anymore I’d just cp them from the source.

To quote them from our IRC channel:

ideally they wouldn't be installed with the wheel
hopefully the upstream will stop using data_files and adapt to the newer workflows

[eoyilmaz]

I'm following @FFY00 's suggestion, he says that data_files is old don't use it. And I'm doing that.

In this page:

https://setuptools.pypa.io/en/latest/userguide/datafiles.html

it recommends using the include_package_data=True keyword, which will allow the data files listed in the manifest.in file to be included in the package. And in its current form, setup.py is spitting out all the entries in the data_files to the manifest.in file.

Now, it might not be solving your issues at all. So, let me reproduce your problem and see if I can find a solution here on my development computer.

As far as understand you are running the following command:

python -m build --wheel --no-isolation
python -m installer --destdir="${pkgdir}" dist/*.whl

What is the value of the $pkgdir environment variable?

[ArchangeGabriel]

OK, I admit again python packaging is far beyond my knowledge, and @FFY00 seems away tonight, so…

Yes, those are the correct step, and technically $pkgdir=/build/displaycal/pkg/displaycal/ in this case, but it does not really matter. It can be any folder you want, and then you should see below it the standard Linux filesystem hierarchy.

[FFY00]

This seems worse and installed even more files into the Python location.

That is the correct behavior. Files cannot be installed to an arbitrary location when using wheels, so you'll have to include it in the package source and have the users copy them to the right location if they want, or do it automatically at runtime. Downstream packagers should copy the files to the right location in their packages.

Yes, those are the correct step, and technically $pkgdir=/build/displaycal/pkg/displaycal/ in this case, but it does not really matter. It can be any folder you want, and then you should see below it the standard Linux filesystem hierarchy.

Yeah, it's just a staging directory. You can do --desdir=staging so that the files aren't copied to the system, but placed in a staging directory instead, tree staging will show the files that would be installed to the system.

[eoyilmaz]

I checked the 55-Argyll.rules files and as you said it exist in ArgyllCMS and I don't see why Florian has included it in DisplayCAL. Maybe it is a convenience for users who are using the internal installation option (the one in the File menu). But, then it can be done at runtime, and I agree that this is not DisplayCAL's responsibility to install. So I'm removing that file, which by the way has a very old content from 2012, the one inside ArgyllCMS is dated 2021 and also has support of i1Pro3 etc.

The other data files will be moved to a suitable folder inside the package and as @FFY00 suggested, installing them at runtime is a very good option, but this will take time.

The setup.py, in its current form, is very cumbersome, and has quite a bit of alternative code paths which are very hard to follow. Hopefully we'll fix it.

Update:
As I see 45-Argyll.rules and 55-Argyll.rules files are there to be installed with the RPM and DEB packages. But, again it is not DisplayCAL's responsibility. If it is not fixing something that is wrong in the upstream ArgyllCMS then they are obsolete in my opinion.

[eoyilmaz]

I updated the setup.py so that the 55-Argyll.rules file will only be included if the bdist_rpm flag is passed to the setup.py command, which we are not using directly anymore...

And if I understand @FFY00 correctly, for your packaging, the placement of the other data files are your responsibility, so good luck with them...

As I said, I'll try to move the data files in to the package and hopefully other than some desktop shortcuts everything will be installed by DisplayCAL in to the correct placement, in the future releases.

If you can confirm that it is okay for you. I'll release the 3.9.5.

[eoyilmaz]

Hmmm... as I see we have a postinstall function inside DisplayCAL.postinstall module, and I see that it is installing icons and desktop shortcuts already.

[FFY00]

And if I understand @FFY00 correctly, for your packaging, the placement of the other data files are your responsibility, so good luck with them...

Yep. We can just place them on the right location during the package building.

Hmmm... as I see we have a postinstall function inside DisplayCAL.postinstall module, and I see that it is installing icons and desktop shortcuts already.

Great!

The setup.py, in its current form, is very cumbersome, and has quite a bit of alternative code paths which are very hard to follow. Hopefully we'll fix it.

I wonder is DisplayCAL would benefit to moving to a more modern build backend like https://github.com/pypa/hatch or https://github.com/FFY00/meson-python.

[eoyilmaz]

I was looking in to Poetry: https://python-poetry.org/docs/

[FFY00]

Don't you need to compile native code?

[eoyilmaz]

I didn't understand you question. Do you imply, if we use Poetry we'll not be able to compile the C-Extension?

[FFY00]

Yes, sorry, I forgot they changed that recently.

[eoyilmaz]

If you can confirm that it is okay for you. I'll release the 3.9.5.

@ArchangeGabriel are you okay with the latest development branch. I'm planning to release 3.9.5 today or may be tomorrow.

[ArchangeGabriel]

Well as I said the build system now install even more files at the wrong place. I prefer the status quo from before (where we had to manually handle 2/3 files) over the changes from 41a6b11, so I’d say let’s revert this commit, but maybe I’m missing something and it was actually required for other cases?

[eoyilmaz]

As far as I see, the only difference is these 4 new files are included in the new tarbal:

ref/XYZ D50.icm
ref/XYZ D50 (ICC PCS encoding).icm
ref/DCDM X'Y'Z'.icm
tests/test_argyll_RGB2XYZ.py

And these are meant to be included. So there is no difference at all. So, I don't think that we need to revert anything and I'm going to release 3.9.5 today or tomorrow.

Thank you.

[ArchangeGabriel]

Those are the difference we see on Arch:

displaycal_package_content_diff.txt

The three ref files seems OK and actually missing from before. Some names are truncated because of the way this is displayed to me, but here is the full package content:

displaycal_diff.txt

As you can see there is a lot of redundancy in the added files.

[eoyilmaz]

Yeah yeah, I just realized that I was wrong, and I was just comparing the tarball content.

[eoyilmaz]

I reverted the change that makes the install_package_data to be True for Linux. And here is the tarball:

DisplayCAL-3.9.5.tar.gz

Please confirm that if it is working for you. Then we can release 3.9.5 and keep working on the build system.

[ArchangeGabriel]

Same issue. I think the whole of 41a6b11 should be reverted. Actually I should be able to test that by patching locally I guess.

EDIT: Confirmed, reverting the whole commit does fix it.

[eoyilmaz]

So, one more question, in the previous method, using setup.py directly, it was installing the data files in to their correct places, right? and now with the wheel, it is not. But because we do not use the include_package_data=True the data files are not going to be present in the wheel, And my question is, are you going to extract them from the tarball and place them manually?

[ArchangeGabriel]

So, one more question, in the previous method, using setup.py directly, it was installing the data files in to their correct places, right?

Yes.

and now with the wheel, it is not. But because we do not use the include_package_data=True the data files are not going to be present in the wheel, And my question is, are you going to extract them from the tarball and place them manually?

Exactly, that’s what @FFY00 said I should do normally (in 3.9.4 I moved them from the wheel to their correct place, but once they are not in the wheel anymore I’ll just install them from the tarball to the right place).

[eoyilmaz]

Okay let's fix your problem, then we should definitely have a more modern build system. I'm reverting back 41a6b11 completely. And I'll do a new 3.9.5 release today.

[eoyilmaz]

Okay 3.9.5 is released: https://github.com/eoyilmaz/displaycal-py3/releases/tag/3.9.5

[ArchangeGabriel]

Sorry for the delay answering here, I had to get this on hold until we could get wxpython 4.1.x into Arch.

Building 3.9.6 resulted in the Argyll rule to disappear (which is fine as we discussed), but the autostart file is still installed, at a wrong place (while we said it should not be automatically installed IIRC). So there is some progress here, but not ideal either.

That being said, I’m fine working around this at packaging time if that is too much work for you to solve. :)

[papoteur-mga]

Hello,
I found that the wrong place for that autostart is determined here:

displaycal-py3/DisplayCAL/setup.py

Lines 712 to 715 in 04c0c49

	autostart
	if os.geteuid() == 0 or prefix.startswith("/")
	else autostart_home,
	[

The problem is that prefix is void. I don't know why, probably because of how the setup.py is called.
In our package build, the user is not root, thus userid is not 0. And as the prefix is no set, it doesn't start with "/".

[papoteur-mga]

But this has no effect on the destination :/

[eoyilmaz]

if the conditions are matching what you given then autostart_home is going to be picked instead of autostart