Added the ability to add extra yum deps to the feedstock.

[pelson]

This PR enables the recipe/yum_requirements.txt capability. Crucially, unlike #135 it puts the yum requirements explicitly into the run_docker_build.sh at conda smithy rerender time, rather than inspecting the yum_requirements.txt at build time.

---

The following content is out of date:

With this change, one can add the following to a conda-forge.yml and expect their build script to add appropriate yum build dependencies:

+++ b/conda-forge.yml
@@ -4,3 +4,6 @@
+  extra-yum-pkgs:
+    - mesa-libGL-devel

There is no guarantee that this is a usable build unless the package is installed on a user's machine also, so this capability should be used exceptionally scarcely.

[pelson]

Ping @jakirkham

[jakirkham]

So, I would feel a little bit better about this if we could handle this file in staged-recipes somehow.

[jakirkham]

Related ( conda-forge/conda-forge.github.io#82 ).

[jakirkham]

specificall -> specific ?

[pelson]

Thanks.

[jakirkham]

Sorry, maybe I'm missing something here. What is the birds eye view here?

[pelson]

Sorry, maybe I'm missing something here. What is the birds eye view here?

The maintainer keeps this information in their conda-forge.yml (the extra-yum-pkgs), and as a result of that, conda smithy rerender gives an appropriate docker script which installs the dependency.

[pelson]

So, I would feel a little bit better about this if we could handle this file in staged-recipes somehow.

I feel the pain, but we don't currently have any ability to change how something is built in staged-recipes (it is all done in a single process as we need runtime determination of what gets built). In the feedstock this is at render time, and so we can control other things such as the docker image used etc.

I completely agree that it is problematic, and I can even see solutions to the problem, I just don't currently believe it is a good use of our time to try to go down that route (I believe it needs a reasonably large change to staged-recipes to get it right) for the limited cases we currently know about.

[jakirkham]

I'm not convinced that this is so bad.

We can add a file to the recipe say yum_requirements.txt (to borrow from pip 😉) and run something like this.

test -f yum_requirements.txt && yum install -y $(cat yum_requirements.txt) || true

I'm not too worried about cross contamination within a PR (at staged-recipes), but we could do something like this after each recipe.

test -f yum_requirements.txt && yum remove -y $(cat yum_requirements.txt) || true

The yum_requirements.txt might look like this for a Qt package.

libXext
libXrender
libSM
tk
libX11-devel

[pelson]

I'm not convinced that this is so bad.

I guess you are right - we know we are only dealing with Linux after all.

So your proposal is to have 1 mechanism for both staged-recipes and feedstocks? Or a different mechanism (with the same result)? How do you want to take this forward @jakirkham - do you want to lead on it, or would you like me to?

[jakirkham]

I guess you are right - we know we are only dealing with Linux after all.

Yep, certainly better than the hacks people do now.

So your proposal is to have 1 mechanism for both staged-recipes and feedstocks? Or a different mechanism (with the same result)?

Basically, I am looking at Linux one liner. Would need to PR it to conda-smithy and staged-recipes.

How do you want to take this forward @jakirkham - do you want to lead on it, or would you like me to?

I'm already toying with this locally. I can let you know when I have something worthy of review.

[jakirkham]

The staged-recipe proposal can be seen in this PR ( conda-forge/staged-recipes#346 ). A sister proposal will be made for conda-smithy.

[pelson]

@jakirkham - I wanted this to be defined at conda-smithy render time, rather than at build time. I can certainly see advantages for doing it at build time (i.e. lack of redundancy) but I wanted this to be as consistent with the rest of the smithy approach, and wanted the dependencies to be much more explicitly defined in the run_docker script.

I'm happy to talk about this further if you wanted to argue for the alternative approach taken in #135.

[pelson]

P.S. One big advantage is that this will not introduce noise to all of the feedstocks when re-rendering, whereas #135 would.

[jakirkham]

I disagree with the implication that PR ( #135 ) is introducing noise. What I liked about that approach, which is why I liked it in staged-recipes, is that it both provides an easy way to install yum requirements and document them. I think those are at least equally important or possibly the latter is more important.

Imagine for an instance you are an end user downloading one of these packages into a docker container. This is becoming an increasingly common use case so is totally reasonable. When you try to download this package, it doesn't work and fails with some strange error. So, you go back to the feedstock to see why this happens. While there is information about this it is not immediately obvious where this is. Perhaps you check the recipe as you know about how conda recipes work, but it is not there. Unless you know to look there, you don't find it or at least it will take awhile to find. In that time, you may start searching the web for a fix, raise an issue on the feedstock, and/or stop using the package altogether. All the while you have become frustrated that this information was either not clearly provided or the package is broken.

That is hardly the experience I would want any user to have with any package here. The advantage of having a simple yum_requirements.txt file is that it documents this information in a clear and straightforward manner and puts it in a place that someone could easily find it even if they knew nothing about conda-forge.

Let's take another use case. Imagine you are trying to get a package out the door at staged-recipes. It turns out it has some involved requirements that reviewers agree is to onerous to address for now, the benefit of getting the package out quickly is too great, and that dropping Linux support seems to extreme. The reviewers agree to let you add yum_requirements.txt for the interim. However, they want these dependencies packaged over the long term and yum requirements to be mostly removed if not dropped completely. If we render this somewhere else in the feedstock, the reviewers need to explain to the maintainer where this will be. As this is not (or should not be) an often used feature, reviewers may forget to explain this and the maintainer might assume it stays with the recipe (reasonable assumption), but not find it there and have to go hunting for where this information lives. The reviewers could also make the same mistake as they may not be familiar with the process.

Eventually, the maintainer (possibly with the help of the reviewers) find this information, but it lives in possibly several places. One of them will be a CI file for sure and will likely be the first thing that's get edited. However, these removed yum requirements will simply get added back when it is re-rendered. As it is extremely unlikely this causes a failure and we often have so many re-rendering PRs that we are rapidly trying to merge them manually, it is very likely that this change gets overlooked entirely and added back in. The provides the possibility for this package or its dependencies to regress as we may be going back to the system versions.

In short, I really still feel that yum_requirements.txt provides the right amount of information with no additional noise. The information it provides is easy to find and easy to index. By keeping it localized to one file, we avoid various potential bugs and mismatched information filling feedstocks. As it should be a seldom used feature, keeping the staged-recipes solution and feedstock solution the same makes it easier to follow and use in the few cases where it is needed.

[pelson]

Phew, long message @jakirkham 😄 .
I can't do that message justice this morning, but thank you for raising your concern. Rather than invest more time in comment ping-pong, let's have a chat on hangouts sometime this week. What days can you do 1400 UTC this week (i.e. the same time we have been doing other conda-forge hangouts)?

[pelson]

Once we've resolved this @jakirkham, I will cut a new conda-smithy release.

[jakirkham]

I'm not sure I have time for another meeting this week. Need to work on some time sensitive internal stuff this week. Maybe we can discuss it in next week's meeting? I think it is on the agenda anyways.

While the yum requirements are important, I don't think it is a crisis if we don't solve it this week. The main problems seem to be centered around VTK (that's why freeglut was added) and this really depends on how quickly that PR is moving. Right now it seems that PR might slow down a bit. If you think it is really urgent to have a release, we could cut it without this. What do you think?

[pelson]

If you think it is really urgent to have a release, we could cut it without this. What do you think?

Sure. Will do.

Thanks @jakirkham!

[jakirkham]

Sorry we couldn't address this sooner.

[pelson]

Sorry we couldn't address this sooner.

Not to worry. It is good to have a need to implement something, so it will healthy when we are under a bit more pressure to achieve it 😄

[pelson]

I disagree with the implication that PR ( #135 ) is introducing noise.

It is adding noise. > 99% of our recipes do not need this, yet every single feedstock will be made to adopt the change. The argument being put forwards is that the additional capability outweighs the noise. +1 (to this PR for not introducing noise)

provides an easy way to install yum requirements

Both PRs provide the recipe/yum_requirements.txt capability +0

Eventually, the maintainer (possibly with the help of the reviewers) find this information, but it lives in possibly several places. One of them will be a CI file for sure and will likely be the first thing that's get edited.

The argument here is about the redundancy. I have addressed that by making the comment in the CI extremely clear:

# Install the yum requirements defined canonically in the
# "recipe/yum_requirements.txt" file. After updating that file,
# run "conda smithy rerender" and this line be updated
# automatically.
yum install pkgA pkgB

In many ways this improves the likelyhood that the user will find yum_requirements, though I accept we can maintain such a comment for the non-rendered form. +0

There is duplication though, so if somebody were to edit this file (which of course they explicitly shouldn't), the changes will be lost when the recipe is rerendered. -1 (for this PR)

The same argument can be made for other variable content which is rendered from conda-smithy. For instance, suppose I wanted to depend on numpy in my recipe, but originally it had not - naively I would just update the run_docker_build.sh to include the numpy matrix and get what I wanted. Unfortunately this would almost certainly be modified when rerendered. As we know, the correct solution is simply to conda smithy rerender and have the scripts updated automatically. I see this consistency a huge boon for maintainablity. +1 (for this PR)

---

So, in conclusion, I still lean towards this PR over the other. @jakirkham - would you mind taking another look at the implementation detail in this PR? I truly believe this to be a healthy way forwards for conda-smithy and want to understand any more arguments against this approach.

[pelson]

@jakirkham - I just wanted to point out that as a result of your excellent PR in #135, I adopted your recipe/yum_requirements.txt into this PR. Unfortunately I didn't update the PR's description, so I think there has been some confusion about the proposed changes. The only difference between this and #135 is that the inspection of yum_requiremens.txt happens at conda smithy rerender, rather than at recipe build time. This is consistent with other features such as the inspection of recipe dependencies, which also impact the CI scripts at conda smithy rerender time, rather than build time.

[jakirkham]

Phew. Long comment. Also, it looks like you have rethought some things since we last chatted. 😉

Will need some time to look over all of this. I'll have to table a response to this until next week.

Are we ok with letting it sit a little longer? Though I think we should spend a little time considering this. After all we can't put Pandora back in the box.

[pelson]

After all we can't put Pandora back in the box.

Actually, we can if we merge this first, then discuss #135. The only feedstocks which would be impacted (by the run_docker_build.sh changing) would be the ones with yum_requirements.txt, which is 0 at the moment, and is likely to be less than a handful for a number of weeks.

Also, it looks like you have rethought some things since we last chatted. 😉

Indeed. I'm sorry about that - I don't like doing it because we did agree to put this behind us. I should have done some more preparation before we met. I'm very sorry about the flip-flopping.

[jakirkham]

Actually, we can if we merge this first, then discuss #135. The only feedstocks which would be impacted (by the run_docker_build.sh changing) would be the ones with yum_requirements.txt, which is 0 at the moment, and is likely to be less than a handful for a number of weeks.

Personally, I don't mind waiting until next week. Sometimes decisions like these take time and that's ok.

Though, if you feel a need to move on this, I still stand behind my work on #135. From my limited understanding of what changed in #134, it should be painless to switch to it and should be incorporated in a re-rendering.

Indeed. I'm sorry about that - I don't like doing it because we did agree to put this behind us. I should have done some more preparation before we met. I'm very sorry about the flip-flopping.

No worries. It's totally ok to rethink things. I'm sure we will do this many times. All I meant was now I also need to think about this too. 😉

[pelson]

So, in order to unblock conda-forge/staged-recipes#568 I will merge this. My rationale is as follows:

• it doesn't tie us into a way of doing things (well, short of having a recipe/yum_requirements.txt, which we both agree on)
• impact of backing out of the change is far smaller than it would be with Provide a mechanism for installing/documenting yum requirements (Pt. 2) #135
• we aren't closing the door on Provide a mechanism for installing/documenting yum requirements (Pt. 2) #135 - it is a supplementary change that we can add at a later date (even if that means next week)

The huge downside for me about doing this though is that it is my PR rather than yours which is being merged - I really don't want this to be seen as a slight on your PR which has in no small part influenced where this PR has ended up.

There are a number of changes backing up in conda-smithy, and I'm going to get them out of the door ASAP. Happy to release again in short order (as has been done before) if new features are introduced.

[jakirkham]

So, in order to unblock conda-forge/staged-recipes#568 I will merge this. My rationale is as follows:

• it doesn't tie us into a way of doing things (well, short of having a recipe/yum_requirements.txt, which we both agree on)

👍

• impact of backing out of the change is far smaller than it would be with Provide a mechanism for installing/documenting yum requirements (Pt. 2) #135

Not sure. Seems about the same.

• we aren't closing the door on Provide a mechanism for installing/documenting yum requirements (Pt. 2) #135 - it is a supplementary change that we can add at a later date (even if that means next week)

That's fine.

The huge downside for me about doing this though is that it is my PR rather than yours which is being merged - I really don't want this to be seen as a slight on your PR which has in no small part influenced where this PR has ended up.

No worries. It seems we agree on the big picture. I have to think about the different details. Again this won't happen until next week.

There are a number of changes backing up in conda-smithy, and I'm going to get them out of the door ASAP. Happy to release again in short order (as has been done before) if new features are introduced.

👍