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 pam-devel and its dependencies pam, audit-libs, cracklib, cracklib-dicts, and libpwquality #55

Merged
merged 23 commits into from
Sep 17, 2024

Conversation

thomann
Copy link
Contributor

@thomann thomann commented Oct 9, 2021

Please see the repo readme for directions on how to make PRs on this repo.

Checklist:

  • if you have added a CDT, it appears in the cdt_slugs.yaml file and
    you have rerun the script python gen_cdt_recipes.py.
  • if you have changed the CDT generator code (rpm.py), you have bumped
    the build number in conda_build_config.yaml and have remade all of the
    recipes via running python gen_cdt_recipes.py --force
  • if you have added a custom CDT recipe, you have added the name of the CDT
    with custom: true in the cdt_slugs.yaml file.
  • all CDT recipes have build number set by {{ cdt_build_number }} for
    old-style/legacy CDTs or {{ cdt_build_number|int + 1000 }} for new-style CDTs
  • if you see a warning about a CDT not having a license, you have added the
    license_file key in the cdt_slugs.yaml file with the path to the appropriate
    license in licenses/

NOTE: If you make any changes to cd_slugs.yaml, you need to reun the generator code
via python gen_cdt_recipes.py.

@thomann
Copy link
Contributor Author

thomann commented Oct 9, 2021

Hello, we need this to add RStudio as a conda-forge package conda-forge/staged-recipes#13760

@isuruf
Copy link
Member

isuruf commented Oct 9, 2021

Can you explain why these cannot be conda packages?

@isuruf
Copy link
Member

isuruf commented Oct 9, 2021

I mean why do they have to be CDTs instead of regular conda packages?

@thomann
Copy link
Contributor Author

thomann commented Oct 9, 2021

Thanks @isuruf for the quick answer!

I'm not sure - they are cdt's in pkgs/main and we used them as such before conda-forge dropped the defaults channel last month.

You think they would better be conda packages?

@isuruf
Copy link
Member

isuruf commented Oct 9, 2021

You think they would better be conda packages?

I have no idea. Sorry. I thought you would have an idea.

@thomann thomann changed the title Add pam-devel and audit-libs Add pam-devel and it's dependencies pam, audit-libs, cracklib, cracklib-dicts, and libpwquality Oct 10, 2021
@thomann
Copy link
Contributor Author

thomann commented Oct 10, 2021

@isuruf I'm not whether it is (easy enough) possible to implement pam-devel and it's dependencies directly as conda-packages. It used to be usable as cdt() until a month ago, maybe then it is ok? Probably the cdt-builds team has to decide.

(bringing @h-vetinari into the loop)

@h-vetinari
Copy link
Member

@thomann: @isuruf I'm not whether it is (easy enough) possible to implement pam-devel and it's dependencies directly as conda-packages.

Are there some guidelines how to determine cdt-vs-regular packages? The documentation is a bit sparse on that, but I guess it's mostly around things having to do with libGL?

Would you prefer to attempt to package these things as regular packages, @isuruf?

@thomann: It used to be usable as cdt() until a month ago, maybe then it is ok? Probably the cdt-builds team has to decide.

Is there anyone else we should be bringing into this discussion, @isuruf?

[Sidenote: Isuru is one of the key people in conda-forge, so he'll likely be involved in such decisions, if he wants to. 🙃]

@thomann: (bringing @h-vetinari into the loop)

Thanks!

@beckermr
Copy link
Member

So cdts are for things where building in conda is very impractical / difficult or we always want to use the system version of a package.

The basic guideline is to never use a CDT unless you absolutely have to.

When I moved the CDT builds from defaults to here, I made CDTs for everything we used at the time. Defaults must have added more in the meantime and we didn't notice until we dropped the channel as you say.

I'd ask you all to take a close look at the underlying software before we merge this and figure out why it is hard to build or w/e.

Maybe the anaconda folks have some insight too? @chenghlee?

cc @conda-forge/core for viz

@chenghlee
Copy link

If I remember correctly, we decided on a libpam CDT for defaults because:

  1. convenience; and
  2. PAM interacts with pretty heavily with the underlying system configuration and security, and at the time, we didn't have the resources to work through how to build conda package(s) in a safe and responsible way.

@beckermr
Copy link
Member

@isuruf what do you think?

@h-vetinari
Copy link
Member

@isuruf: I mean why do they have to be CDTs instead of regular conda packages?

To find out (and hopefully move forward one way or another), I opened conda-forge/staged-recipes#16911, though I'm pretty quickly reching the limits of my make-fu.

I also think the following comment above is pretty pertinent if we want to consider packaging it ourselves:

@chenghlee: PAM interacts with pretty heavily with the underlying system configuration and security, and at the time, we didn't have the resources to work through how to build conda package(s) in a safe and responsible way.

@sshockwave
Copy link
Member

sshockwave commented Feb 4, 2023

Hi everyone. Please excuse me for bringing up an old issue. I had another attempt on the regular packaging: conda-forge/staged-recipes#21955. The build is "passed" from a tech perspective, but I don't have the expertise to spot the security issues. I would appreciate some reviews and comments!

@h-vetinari
Copy link
Member

Quoting from conda-forge/staged-recipes#21955 (respin of conda-forge/staged-recipes#16911), we finally seem to have direction to move forward with this:

@carterbox: @sshockwave, I talked with some of the core team members at the community meeting today. It seems that a majority would prefer that this package is shipped as a CDT.

As a side note, there were many questions about what downstream conda packages would need to link to libpam (i.e. what is the utility of this package for conda) and even whether this non-CDT libpam package would function on non-selinux distros. Please be prepared to answer these types of questions, so the core team can help package this library appropriately.

Also copying my answer w.r.t. to the last bit:

The main reason we started down this road was to package rstudio-server, I'm not currently aware of other packages that require this.

@h-vetinari
Copy link
Member

Ah, just saw a linked PR above that also seems to need it: jupyter-desktop-server...

@beckermr
Copy link
Member

I think we're resolved to merge this once it passes and I have a chance to do some review.

@beckermr
Copy link
Member

Thanks everyone for working through this. A full year is not great for a pr but we got to the end!

@isuruf
Copy link
Member

isuruf commented Feb 23, 2023

I think we're resolved to merge this once it passes and I have a chance to do some review.

I thought the consensus was that we needed more information either way.

@h-vetinari
Copy link
Member

This came up in the core call in context of #66 today. I brought it as an example of something that should be a CDT, not least due to the security implications and infeasibility of properly testing this. There was general agreement on having libpam as a CDT (particularly from @mariusvniekerk, who seems to know libpam quite well), and no disagreement.

So I'd like to unblock this PR, independently of the alma8 enablement effort.

@zmbc
Copy link

zmbc commented Jun 6, 2024

Don't want to add noise here, but I would really appreciate a rstudio-server conda package and was surprised to see that it was blocked by this. It sounds like there is broad agreement that this should be a CDT. What are the next steps to get this merged?

@h-vetinari h-vetinari changed the title Add pam-devel and it's dependencies pam, audit-libs, cracklib, cracklib-dicts, and libpwquality Add pam-devel and its dependencies pam, audit-libs, cracklib, cracklib-dicts, and libpwquality Sep 15, 2024
@h-vetinari h-vetinari closed this Sep 15, 2024
@h-vetinari h-vetinari reopened this Sep 15, 2024
@h-vetinari
Copy link
Member

@beckermr, the builds were hanging and I managed to solve this by adding a swapfile, except for the two altarch legacy jobs: cos7_{aarch64,ppc64le}_legacy, which seem to hang on

building recipes:   0%|                                 | 0/175 [11:43<?, ?it/s]

even with 30GB swapfile. Do we still need the legacy CDTs? If so, would you know how to debug/fix what's happening?

@beckermr
Copy link
Member

We can drop all of the cos6 and legacy cdt builds.

@h-vetinari
Copy link
Member

So I cleaned up this long-standing PR (we had agreed to make pam a CDT in a core-call in March), which needed a bit of work because the CentOS 7 repos aren't online anymore. While we're at it (and on @beckermr's hint), I also dropped cos6 & legacy CDTs. Plus some clean-ups w.r.t. skips, configs & scripts. Given the large mechanical changes, it's probably easiest to review per commit.

I have some follow-up work to add some (limited) Alma 8 CDTs on top of this.

PTAL @conda-forge/core

@beckermr beckermr self-requested a review September 16, 2024 22:54
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

We may have to restart the build here a few times to get it to pass, but LGTM!

@h-vetinari h-vetinari merged commit fb6bb60 into conda-forge:main Sep 17, 2024
4 checks passed
@h-vetinari h-vetinari mentioned this pull request Sep 17, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants