389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
210 stars 90 forks source link

Fix GitHib CI 'cannot find -lldap_r' issue #5339

Open droideck opened 2 years ago

droideck commented 2 years ago

Issue Description The error 'cannot find -lldap_r' happens when GitHub CI is run on a non-master branch which doesn't have the https://github.com/389ds/389-ds-base/commit/a3375a08a506c3b72a8a23df5238848017691350 fix.

The issue happens if you build older branches on Fedora 36 (where we have OpenLDAP 2.6).

We should be careful with backporting the fix as we may find ourselves in the "dangerous" territory when we build 389-ds-base with libdap from openldap-2.6, but it will be used with openldap-2.4.

We shouldn't have issues on Fedora 35, but we may get issues on RHEL 8 or SUSE if they don't have libldap/libldap_r fix from https://fedoraproject.org/wiki/Changes/OpenLDAPwithoutNonthreadedLibraries

So we should always build with libldap_r the package that should be used on a system where we use libldap_r.

So we may need to find another way to fix the github CI. Simple cherry-pick to older branches may cause problems because of the issue described above.

droideck commented 2 years ago

Also, it may not be worth the hustle as we don't usually run CI on anything besides master. The rare cases we need it, we can do it manually and check it on the correct OS (RHEL, SUSE, older Fedora).

progier389 commented 2 years ago

Also, it may not be worth the hustle as we don't usually run CI on anything besides master.

@droideck, I am afraid that you are wrong on that point: I received the test report failures after having cherry-picked a fix and pushed it on the github repository. As I understand things the tests are triggered automatically through the github actions defined in https://github.com/389ds/389-ds-base/tree/master/.github/workflows when a commit is merged or pushed on any branches ...

droideck commented 2 years ago

Also, it may not be worth the hustle as we don't usually run CI on anything besides master.

@droideck, I am afraid that you are wrong on that point: I received the test report failures after having cherry-picked a fix and pushed it on the github repository. As I understand things the tests are triggered automatically through the github actions defined in https://github.com/389ds/389-ds-base/tree/master/.github/workflows when a commit is merged or pushed on any branches ...

Sorry, mate, I wasn't clear enough on my point. I meant that our GitHub CI is built for testing the main branch. When we need to test 1.4.3, it's better to test it on the targeted platform anyway (SUSE, RHEL, old Fedora, etc.). It's the more correct thing to do.

So personally, as a reviewer, I never ask for a GitHub CI pass when the fix is not from the main branch.

We still can fix the issue, but we should always be careful and test things on the targetted platform mainly, IMHO.

vashirov commented 2 years ago

I meant that our GitHub CI is built for testing the main branch.

Not really, it can run tests from all branches where GH actions workflow yaml files are present. I filed https://github.com/389ds/389-ds-base/issues/5309 to sync yaml files across branches to fix this. I'll file PRs to the relevant branches this week.

When we need to test 1.4.3, it's better to test it on the targeted platform anyway (SUSE, RHEL, old Fedora, etc.). It's the more correct thing to do.

I disagree. We can and should run automated tests in upstream for older branches, it's free and gives feedback before the code is tested in downstream. We've seen multiple issues in downstream recently when some lib389 or tests commits were not backported, and it was only found when tests were broken.

We can also explore the option to test older branches on CentOS Stream instead of Fedora. We can't do both as the test matrix starts to hit the current GitHub Actions limits.

Firstyear commented 2 years ago

I disagree. We can and should run automated tests in upstream for older branches, it's free and gives feedback before the code is tested in downstream. We've seen multiple issues in downstream recently when some lib389 or tests commits were not backported, and it was only found when tests were broken.

Yeah, I think this is a good idea.

droideck commented 2 years ago

I meant that our GitHub CI is built for testing the main branch.

Not really, it can run tests from all branches where GH actions workflow yaml files are present. I filed #5309 to sync yaml files across branches to fix this. I'll file PRs to the relevant branches this week.

When we need to test 1.4.3, it's better to test it on the targeted platform anyway (SUSE, RHEL, old Fedora, etc.). It's the more correct thing to do.

I disagree. We can and should run automated tests in upstream for older branches, it's free and gives feedback before the code is tested in downstream. We've seen multiple issues in downstream recently when some lib389 or tests commits were not backported, and it was only found when tests were broken.

We can also explore the option to test older branches on CentOS Stream instead of Fedora. We can't do both as the test matrix starts to hit the current GitHub Actions limits.

Okay, sounds good to me! And I admit that I was a bit harsh in my suggestion. Also, we've discussed most of it in the recent meeting, and yes, I agree that we need to improve our CI (as was my initial intention when I created the issue).

Firstyear commented 2 years ago

No stress :)