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

add Arch Linux compatibility for OS dependency resolution #4116

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

LukeLabrie
Copy link

@LukeLabrie LukeLabrie commented Oct 30, 2022

per issue #833, added some logic to accommodate some archlinux distros (currently just arch and manjaro) when checking for os dependencies. some notes:

  • should work for any arch distro that uses pacman as long as it's added to the arch_distros list
  • i had to specifically define ARCH_OS_DEP_CONSTANTS because an import of EASYCONFIG_CONSTANTS from easybuild.framework.easyconfig.constants results in a circular import... perhaps there's a better way to do it
  • the names of the packages need to be changed when using those constants, since they have different names on arch. arch packages typically don't have the "...-dev" suffix, but it seems like the arch packages incorporate the dev features from other distros. I don't think you can just add the names to EASYCONFIG_CONSTANTS because then you might get the wrong version on other distros

works for me locally... let me know your thoughts

@@ -211,6 +212,19 @@
'setuptools': ('pkg_resources', "obtaining information on Python packages via pkg_resources module"),
}

# os dependency constants that can be used in easyconfig (for name updates on arch distros)
ARCH_OS_DEP_CONSTANTS = {
'IBVERBS_DEV': (('libibverbs-dev', 'libibverbs-devel', 'rdma-core-devel'),
Copy link
Member

Choose a reason for hiding this comment

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

@LukeLabrie We already have constants for this in easybuild/framework/easyconfig/constants.py, we shouldn't copy that here.

If the package names for openssl & co are slightly different on Arch, then we should just add an item to the tuples in easybuild/framework/easyconfig/constants.py, since the tuples specify alternatives for the package names that are considered.

So for example, adding rdma-core to the tuple for OS_PKG_IBVERBS_DEV:

EASYCONFIG_CONSTANTS = {
    ...
    'OS_PKG_IBVERBS_DEV': (('libibverbs-dev', 'libibverbs-devel', 'rdma-core-devel', 'rdma-core'),
                           "OS packages providing ibverbs/infiniband development support"),

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback @boegel. To your first comment, I tried that initially but since easybuild/framework/easyconfig/constants.py imports easybuild/tools/systemtools.py importing the constants from constants.py results in a circular import -- but as I mentioned I'm sure there's a cleaner way to avoid this so I can look into that.

To your second comment, I also tried that initially, but wouldn't that create an issue on distros where there is a meaningful difference between the names rdma-core and rdma-core-devel? For example, I think these would be two different packages on redhat distros, so adding rdma-core to the tuple would mean a redhat user would pass a dependency check even if they didn't have rdma-core-devel. That's the only reason I added specific logic for arch distros.

Copy link
Member

Choose a reason for hiding this comment

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

Right, that's a good point on rdma-core-devel vs rdma-core.

I guess that forces us to do the override you're suggesting for Arch/Manjaro, at least with the current approach.

It's probably a good idea to add a comment above the code block you're currently adding to constants.py, since this complication is not immediately obvious, just to avoid that someone else goes "how silly, let's just add rdma-core to the tuple instead"...

@boegel boegel added this to the 4.x milestone Nov 9, 2022
@boegel boegel changed the title add archlinux compatibility for os dependency resolution add Arch Linux compatibility for OS dependency resolution Nov 9, 2022
@boegel boegel modified the milestones: 4.x, next release (4.6.3?) Nov 10, 2022
@LukeLabrie
Copy link
Author

@boegel, per your feedback I added a comment to the codeblock in constants.py to explain why it can't simply be added to the tuples. However after some testing, I found that given the current approach it is still necessary to define the package names that require modification in systemtools.py, even though they're already defined in constants.py. The logic in constants.py is fine so long as the easyconfig uses the predefined easyconfig constants for the os dependencies. But it is fairly common for the dependencies to instead be defined explicitly. For example in easybuild-easyconfigs/easybuild/easyconfigs/p/Python/Python-3.8.2-GCCcore-9.3.0.eb, we have

osdependencies = [('openssl-devel', 'libssl-dev', 'libopenssl-devel')]

(which could have equivalently been written as 'OS_PKG_OPENSSL_DEV') in which case the dependencies will be fed directly into the check_os_dependency() function in systemtools.py without modification from constants.py. In the above commits, I've tried to do accommodate for this in a minimal way.

I think that if you want the package names (that will need to be changed for arch distros) to be defined in only one place (like constants.py), there needs to be some refactoring such that constants.py does not depend on systemtools.py. Let me know your thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants