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

netatalk: Update to 4.0.4 #25200

Merged
merged 3 commits into from
Nov 3, 2024
Merged

netatalk: Update to 4.0.4 #25200

merged 3 commits into from
Nov 3, 2024

Conversation

APCCV
Copy link
Contributor

@APCCV APCCV commented Oct 25, 2024

Maintainer: Antonio Pastor / @APCCV
Compile tested: ipq806x (23.05.3, snapshot)
Run tested: ipq806x - C2600: start server, connect from MacOS, read/write files to home share, create/update TimeMachine backups

Description:
No changes to package other than using latest available upstream code base. Starting Netatalk 4.x build uses meson instead of autotools.

net/netatalk/Makefile Outdated Show resolved Hide resolved
Copy link
Member

@BKPepe BKPepe left a comment

Choose a reason for hiding this comment

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

So many unrelevant changes, which are not described anywhere.

  1. Downloading subpackage
  2. Adding indentation, which then makes it completely useless.
    etc

@APCCV
Copy link
Contributor Author

APCCV commented Oct 25, 2024

Updated indents. Comments on your change above.

As the commit message states, upstream moved from autotools to meson. That is the bulk of the changes.

The other changes are my attempt to clean up this old Makefile. Looked at other Makefiles in different libraries to match as best as it made sense to me.

Added a download for a pre-requisite. Not a sub-package.
Upstream removed a file (Unicode DB) from the tarball and made it a pre-req. Reference: https://github.com/Netatalk/netatalk/releases/tag/netatalk-4-0-0

Did not find any package that provides this file. Makes sense as it is only used during build. At 2.2MB, it is quite large so I considered it to be a better solution to download it during build than to store it in GitHub. Having it in Makefile makes it slightly easier to update if a new version of the Unicode DB were to be released.

Added a comment to Makefile for it.

If you think a different approach is needed let me know which (and if you don't mind, why).

@APCCV APCCV requested a review from BKPepe October 25, 2024 13:52
@APCCV APCCV changed the title netatalk: Update to 4.0.2 netatalk: Update to 4.0.3 Oct 27, 2024
@APCCV
Copy link
Contributor Author

APCCV commented Oct 27, 2024

4.0.3 just released. Updated version and download hash. Renamed PR.
Everything else remained the same regarding 4.0.2 PR.

@Neustradamus
Copy link

@APCCV: Very fast update, thanks :)

Copy link
Member

@BKPepe BKPepe left a comment

Choose a reason for hiding this comment

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

Cleaned up and reordered Makefile to better match layout of other projects

Should be done in separate commit to be more readable as there is so many changes related to meson. = Increases readability, easy to revert.

Added a download of a file (Unicode DB, UnicodeData.txt) that is used during compilation.

Do it in separate Makefile, please. = Hopefully, in the future, more packages will use it. Its logical to have it separately, because I am not quite sure why do you need to download it as subpackage if multiple GNU/Linux distributions such as Debian have that file in its own package. E.g. https://packages.debian.org/sid/unicode-data

  • How would you even version downloaded unicode?

net/netatalk/Makefile Outdated Show resolved Hide resolved
net/netatalk/Makefile Outdated Show resolved Hide resolved
net/netatalk/Makefile Outdated Show resolved Hide resolved
@APCCV
Copy link
Contributor Author

APCCV commented Oct 28, 2024

Should be done in separate commit to be more readable as there is so many changes related to meson. = Increases readability, easy to revert.

Ok.
These changes were not really required but since I was touching it, I thought why not.
If a separate commit is better for it, no problem.

@APCCV
Copy link
Contributor Author

APCCV commented Oct 28, 2024

* How would you even version downloaded unicode?

It can be versioned. I can add an additional variable for version. Should have done that, like this (note UC_DATA_VERS):

# Unicode DB file required for compilation
UC_DATA_FILE:=UnicodeData.txt
UC_DATA_VERS:=16.0.0
UC_DATA_URL:=https://www.unicode.org/Public/$(UC_DATA_VERS)/ucd/
UC_DATA_HASH:=ff58e5823bd095166564a006e47d111130813dcf8bf234ef79fa51a870edb48f

Do it in separate Makefile, please. = Hopefully, in the future, more packages will use it. Its logical to have it separately, because I am not quite sure why do you need to download it as subpackage if multiple GNU/Linux distributions such as Debian have that file in its own package. E.g. https://packages.debian.org/sid/unicode-data

This is where I would rather debate the point.
Yes, many distributions have this file installed already. My first PR, which I cancelled, failed tests because of it. On my computer the package builds because the build finds the file on my distribution, outside the build "sandbox".
I think that is not correct. I think a package build should be self sufficient without requiring files from the host OS.
And again, the OpenWrt build failed because the containers don't have the file as they don't have the https://packages.debian.org/sid/unicode-data package installed (or equivalent for Alpine or whatever they use). Should it be installed there? Can it? For this (small) thing? How does that get done? Do an apt install during Prepare?

You have a point with reuse, but I doubt many other packages would use this. The issue w/netatalk is how the upstream used to have this file as part of the tarball, but now dropped it with no mechanism to download it as part of the build. How does OpenWrt packages normally deal with this? Can you think of an example so I take a look?

I am not to sure what you mean with "a separate Makefile." That's on me, I'm far from an expert with OpenWrt packages.

  • Do you mean creating a separate "UnicodeData" package? That will not work. We don't want this raw file on the routers. We need the compiled version, and the build process does that. New package that downloads this raw file, and make it just a build dependency?
  • Do you mean as a separate build process that is called before the package build to download the file and place it somewhere on the OS? I have no clue how to do that, but I can search and learn if that is the case. It would be awesome if you could point me to an existing package that has two Makefiles. I'll search for one in the meantime.
  • Something else?

Thanks for your time and change requests to clean this up.

PS: never mind... found the file in the SDK. I could have sworn I looked for it there before. It is not the latest, but it's the official one so it's good enough. Will remove the additional download.
I won't create a separate package (Makefile?) for it... tried and failed. Doesn't help we are looking at a text file with no build scripts. OpenWrt doesn't seem to like that.

@APCCV
Copy link
Contributor Author

APCCV commented Oct 29, 2024

Alright. Summary:

  • two commits, as requested, one with change to meson, one with cosmetic stuff
  • eliminated download of additional file so no additional makefile or package

Please take a look.

@APCCV APCCV requested a review from BKPepe October 29, 2024 03:02
@BKPepe
Copy link
Member

BKPepe commented Oct 29, 2024

  • Do you mean creating a separate "UnicodeData" package? That will not work. We don't want this raw file on the routers. We need the compiled version, and the build process does that. New package that downloads this raw file, and make it just a build dependency?

Yes, I mean to create that package and mark it as BUILDONLY:=1 e.g. how it is done in nacl Makefile in this repo.

Since you found out the file in the build system, then great! :) I havent checked it myself so far. I would do that if I there is need to create a new Makefile for the unciode.

net/netatalk/Makefile Outdated Show resolved Hide resolved
Copy link
Member

@BKPepe BKPepe left a comment

Choose a reason for hiding this comment

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

Otherwise, I really appreciate that you weren’t afraid to tackle this, found a solution on your own, and even tested it to see if it actually works. This is something you don’t see often these days, as people tend to copy things from the Internet without trying them out. It’s clear that you have patience and determination. This approach is rare and truly excellent. This will greatly help you in your journey of contributing to OpenWrt. Thank you for your work! :)

@APCCV
Copy link
Contributor Author

APCCV commented Oct 30, 2024

Thank you for your work

Thank you for your patience.

Since the PR has not been merged yet, I'm committing an additional change to enable the old AppleTalk protocol. I held it back because it needed patching (and I always have problems with patches) or waiting for the next release. Against all odds I figured out the patches. I'll remove them when 4.0.4 (or later) rolls out.
AppleTalk is really the key enhancement for v4. It doesn't add to much additional space so I think it's worth including.
I'd be surprised if more than 2 people end up using it. If anyone does, they are absolutely going to love that their OpenWrt router now networks all Macs.

Also fixed that STAGING_DIR_HOST path and removed the extra linefeed at the end of file.

If it's all good to you now, please merge to the repo.

@APCCV APCCV requested a review from BKPepe October 30, 2024 00:29
@APCCV
Copy link
Contributor Author

APCCV commented Oct 30, 2024

Alright. Really done with 4.0.3. Won't touch it anymore.
Please merge if you have no further concerns.

Antonio Pastor added 2 commits November 3, 2024 13:38
Maintainer: Antonio Pastor / @APCCV
Compile tested: ipq806x (23.05.3, snapshot)
Run tested: ipq806x - C2600: start server, connect from MacOS, read/write files to home share, create/update TimeMachine backups

Description:
No changes to package other than using latest available upstream code base. Starting Netatalk 4.x build uses meson instead of autotools.

Signed-off-by: Antonio Pastor <[email protected]>
Updated title.
Updated description.
Reordered to match most common layout

Signed-off-by: Antonio Pastor <[email protected]>
@APCCV APCCV changed the title netatalk: Update to 4.0.3 netatalk: Update to 4.0.4 Nov 3, 2024
@APCCV
Copy link
Contributor Author

APCCV commented Nov 3, 2024

Won't touch it anymore

Famous last words.
Bumped to 4.0.4 as it is now available. Removed patch I had in here before as those changes were adopted upstream.

Please merge if you are OK with PR as it stands now.

@APCCV
Copy link
Contributor Author

APCCV commented Nov 3, 2024

make[1]: *** [package/Makefile:179: package/kernel/linux/compile] Error 1

Aw, come on... TF? I managed to make changes just when the build process is broken? My luck...

Starting v4.0.0, netatalk brings back AppleTalk features lost in v3.
Updated description.

Signed-off-by: Antonio Pastor <[email protected]>
@APCCV
Copy link
Contributor Author

APCCV commented Nov 3, 2024

:(

@BKPepe
Copy link
Member

BKPepe commented Nov 3, 2024

CI/CD failures are not relevant to this PR. Merging, thank you.

@BKPepe BKPepe merged commit 09a2427 into openwrt:master Nov 3, 2024
8 of 13 checks passed
@Neustradamus
Copy link

Good job, thanks to @APCCV, and @BKPepe for merging!

Can be backported to 23.05.x and 24.10.x?

@APCCV
Copy link
Contributor Author

APCCV commented Nov 3, 2024

Can be backported to 23.05.x and 24.10.x?

I have no problem with it.
This package is all userspace... so I think you can take the snapshot package and install in 23.05... haven't tested that yet, but will give that a try.
What I really want is for it to be in the imagebuilder (makes life simpler).

For next release I'm planning on splitting in small/large packages. Anyone having the resources (and an x86) to run the build with everything will probably be better off with the docker image. For smaller devices a -small package with only afpd will be more useful.

So how do we know if I can backport it? Just open the PR?

PS: darn... I really wanted this to make it to 24.x. Missed by... hours?

@APCCV
Copy link
Contributor Author

APCCV commented Nov 4, 2024

Opened PR against 24.10 branch.
#25246

Package did not exist in 23.x, so I am not sure we can add it to that branch at this point. Hopefully the new PR will get comments about it.

@hnyman
Copy link
Contributor

hnyman commented Nov 4, 2024

Buildbot finds new dependencies and 4.0.4 does not compile...
New detected dependencies/features should be either disabled in the package configuration, or the correct dependencies added.

E.g.
https://downloads.openwrt.org/snapshots/faillogs/aarch64_cortex-a72/packages/netatalk/compile.txt

Package netatalk is missing dependencies for the following libraries:
libgio-2.0.so.0
libglib-2.0.so.0
libgobject-2.0.so.0
make[3]: *** [Makefile:122: /builder/shared-workdir/build/sdk/bin/packages/aarch64_cortex-a72/packages/netatalk_4.0.4-r1_aarch64_cortex-a72.ipk] Error 1

Backporting to 24.10 would make sense after it first compiles ok in master...
(but apparently this has already been backported to 24.10. Compilation will fail there too.)

@APCCV
Copy link
Contributor Author

APCCV commented Nov 4, 2024

This was building properly.
Apologies.
I will rebuild from scratch and have a PR to address as soon as possible.

@hnyman
Copy link
Contributor

hnyman commented Nov 4, 2024

This was building properly.
Apologies.

No trouble.
It is just that as the buildbot builds all packages/libraries, there are much more libraries available than in your own build system (or in the PR CI testing). So when some additional features have been introduced in a package's new version, it may happen that new dependencies are detected only in buildbot.
This happens every now and then.

It pays off to monitor the buildbot faillogs after a PR gets merged.

I will rebuild from scratch and have a PR to address as soon as possible.

You just need to either add those detected dependencies (possibly bloating the needed amount of libraries) ot adjust the build configure step of the package to leave those detected libraries unused.

@APCCV
Copy link
Contributor Author

APCCV commented Nov 4, 2024

adjust the build configure step of the package to leave those detected libraries unused

That is my goal. It is relatively clear what is causing the issue.
PR to HEAD first? I can work on it in 12h.

@hnyman
Copy link
Contributor

hnyman commented Nov 4, 2024

Yeah, first to main/master

@BKPepe
Copy link
Member

BKPepe commented Nov 4, 2024 via email

@APCCV
Copy link
Contributor Author

APCCV commented Nov 4, 2024

Submitted #25251

@APCCV
Copy link
Contributor Author

APCCV commented Nov 5, 2024

Where can I see the buildbot log for this? The one asking for those additional libraries?
I assume somewhere here https://buildbot.openwrt.org/master/packages/#/grid but so far I haven't found it.
Any pointers are welcome.


Oh... you were kind enough to provide the log.
CUPS was a concern... the build bot is finding it, but not finding a config, so there are no CUPS driven dependencies breaking this.
I am now more confident than just turning off afpstats will clear this issue. Please merge if OK with this.

@hnyman
Copy link
Contributor

hnyman commented Nov 5, 2024

Where can I see the buildbot log for this? The one asking for those additional libraries?

How about the link I gave above?
to the "faillogs".
All architectures are here:
https://downloads.openwrt.org/snapshots/faillogs/

E.g. these two builds below are from today. But based on timestamps, they are from time before I merged your patch attempt.
https://downloads.openwrt.org/snapshots/faillogs/aarch64_cortex-a76/packages/netatalk/compile.txt
https://downloads.openwrt.org/snapshots/faillogs/mipsel_24kc/packages/netatalk/compile.txt

You may need to wait for tomorrow, as today the whole buildbot infra was upgraded , and the phase2 packages buildbot is not really visible yet.
(Or at least I haven't been able to find it... @ynezz might know more: https://lists.openwrt.org/pipermail/openwrt-devel/2024-November/043358.html )

@APCCV
Copy link
Contributor Author

APCCV commented Nov 6, 2024

How about the link I gave above?

Thank you. Noticed that after I had posted my comment after a long trip through the build bot site. I know where to look now. Should've edited the comment.

You may need to wait

No problem waiting.
Based on timestamps. Build logs for snapshot packages stopped at Tue Nov 5 09:40:44 2024 (for yesterday's batch) to start again at Tue Nov 5 23:26:46.
There's no failed log for arm_arm926ej-s, for example, which ended at Tue Nov 5 23:58:28 it seems.
And package is there: https://downloads.openwrt.org/snapshots//packages/arm_arm926ej-s/packages/netatalk_4.0.4-r1_arm_arm926ej-s.ipk with date Tue Nov 5 21:58:00 2024.

I waited for builds to start succeeding before I created the backport PR. I don't like breaking builds (and I get it's not of much consequence in this case).

Anyways, no rush. Backport PR is there. #25260
Should fix my mess in 24.10. Please merge when you think is prudent to.

@hnyman
Copy link
Contributor

hnyman commented Nov 6, 2024

Thanks, I merged the backport.

@Neustradamus
Copy link

Thanks to @APCCV, @BKPepe for merging and @hnyman for review!

Original PRs in master:

24.10 backport:

@Neustradamus
Copy link

@APCCV, @BKPepe, @hnyman: It is possible to backport for 23.05.x too?

Thanks in advance.

@APCCV
Copy link
Contributor Author

APCCV commented Nov 7, 2024

It is possible to backport for 23.05.x too?

Backporting it would require 6 commits from when I took over the package in June. I can create a PR with that.
---OR---
I can port it by creating a PR with a new commit with the current Makefile.

I created the backport PR with 6 commits. If that's not the way to go please close it and let me know if I should proceed some other way.

@APCCV
Copy link
Contributor Author

APCCV commented Nov 25, 2024

@Neustradamus saw those. I get an email on new releases. rdmark also reached out about 4.0.5 for reasons below.

This is what we have in progress.

Ah, the irony. Honestly it was painful to deal with the Unicode txt file, first to figure how the … to make it work, then to make it OpenWrt compliant with an extra package (almost got there), then to make it work with a file from host (duh - had I known). In the meantime I exchanged notes with rdmark. Turns out OpenWrt wasn’t the only packaging that was going through this particular circle of hell, and now the file is optional.
Beyond that the changes are mostly cosmetic or irrelevant to OpenWrt.

Also has little to offer over 4.0.4 unless using a System 7 client over TCP (which I can’t test).

I’ll keep an eye on releases for something major (in substance, not in version numbering) and will consider updating then. In the meantime I’d rather see 4.0.4 backported to 23.05 (I hope), the AppleTalk module backported to 23.05 and 24.10, and the atalkd problem (on DSA?) narrowed down if not fixed. That’s where time left after paying job and real life will go for a while.

Any particular issues, report as a bug to netatalk. I’m just the messenger. ;)

@APCCV
Copy link
Contributor Author

APCCV commented Nov 25, 2024

Besides... we are making progress. Module is now in snapshot. Check it out:
https://downloads.openwrt.org/snapshots/targets/ipq806x/generic/kmods/6.6.63-1-965d9c45def76b2480c9d5ecf1242091/

That it is not functional because of an issue with it, atalkd or DSA doesn't take away from netatalk making it back into OpenWrt after almost being completely forgotten. 24.10 will have fully working afpd... and maybe atalkd.

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