copy all files as non-executable by default - #2596
Conversation
722622c to
97fd607
Compare
| try: | ||
| import pygit2 | ||
|
|
||
| # ensure we add all files as non-exectuable by default | ||
| entry = repo.index[index_path] | ||
| entry.mode = pygit2.GIT_FILEMODE_BLOB # 100644 (non-executable) | ||
| repo.index.add(entry) | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
Note, I've used the try/except approach here to stay compatible with
conda-smithy/conda_smithy/feedstock_io.py
Lines 10 to 25 in 3e8e768
which seems to intentionally avoid a hard dependence on pygit2
|
@beckerm @jaimergp, would appreciate reviews on this, as this is currently blocking1 the bot from picking up new pinning files, see conda-forge/conda-forge-bot#6237; it looks like another latent bug that got surfaced by ... something. 🤷 Footnotes
|
75e9af9 to
bb9f098
Compare
beckermr
left a comment
beckermr
left a comment
There was a problem hiding this comment.
Here are the permissions of the files on main:
(base) beckermr@mac conda-smithy % ls -lah conda_smithy/templates
total 320
drwxr-xr-x 27 beckermr staff 864B Jun 14 04:27 .
drwxr-xr-x 29 beckermr staff 928B Jun 14 04:27 ..
-rw-r--r--@ 1 beckermr staff 2.9K Dec 21 2023 appveyor.yml.tmpl
-rw-r--r--@ 1 beckermr staff 3.7K Jun 14 04:27 azure-pipelines-linux.yml.tmpl
-rw-r--r--@ 1 beckermr staff 3.1K Jun 14 04:27 azure-pipelines-osx.yml.tmpl
-rw-r--r--@ 1 beckermr staff 5.2K Jun 14 04:27 azure-pipelines-win.yml.tmpl
-rw-r--r--@ 1 beckermr staff 1.8K Apr 18 17:06 azure-pipelines.yml.tmpl
-rw-r--r--@ 1 beckermr staff 8.5K Jun 14 04:27 build_steps.sh.tmpl
-rw-r--r--@ 1 beckermr staff 2.8K Dec 21 2023 circle.yml.tmpl
-rw-r--r--@ 1 beckermr staff 277B Jul 24 2023 conda-forge.yml.ci-skel.tmpl
-rw-r--r--@ 1 beckermr staff 2.9K Jun 14 04:27 create_conda_build_artifacts.bat.tmpl
-rwxr-xr-x@ 1 beckermr staff 4.2K Jun 14 04:27 create_conda_build_artifacts.sh.tmpl
-rw-r--r--@ 1 beckermr staff 934B Jun 14 04:27 create_pagefile.bat.tmpl
-rwxr-xr-x@ 1 beckermr staff 501B Jun 14 04:27 create_pagefile.sh.tmpl
-rw-r--r--@ 1 beckermr staff 1.6K Jun 14 04:27 drone.yml.tmpl
-rw-r--r--@ 1 beckermr staff 34B Jul 24 2023 fast_finish_ci_pr_build.sh.tmpl
-rw-r--r--@ 1 beckermr staff 1.3K Jun 14 04:27 free_disk_space.sh.tmpl
-rw-r--r--@ 1 beckermr staff 13K Jun 14 04:27 github-actions.yml.tmpl
-rw-r--r--@ 1 beckermr staff 2.3K Jul 24 2023 meta.yaml.ci-skel.tmpl
-rw-r--r--@ 1 beckermr staff 3.6K Nov 9 2025 pixi.toml.tmpl
-rw-r--r--@ 1 beckermr staff 13K May 24 08:13 README.md.tmpl
-rw-r--r--@ 1 beckermr staff 5.1K Jun 14 04:27 run_docker_build.sh.tmpl
-rw-r--r--@ 1 beckermr staff 7.9K Jun 14 04:27 run_osx_build.sh.tmpl
-rw-r--r--@ 1 beckermr staff 10K May 11 2025 run_win_build.bat.tmpl
-rw-r--r--@ 1 beckermr staff 6.9K Jun 14 04:27 SetPageFileSize.ps1.tmpl
-rw-r--r--@ 1 beckermr staff 2.5K Apr 18 17:06 travis.yml.tmpl
-rw-r--r--@ 1 beckermr staff 1.2K Jul 24 2023 woodpecker.yml.tmpl
Instead of maintaining separate lists of which files need to be executable, we should set the correct permissions on the template file in the repo itself and ensure those permissions are preserved after the file is rendered.
why? This is exactly the reason why we find ourselves in this situation. The code in I find it much simpler to just clear the exec bit when copying; we already require all the machinery for setting the executable bit anyway, and we already have a list of all templates in Explicit is better than implicit. And making sure the executable bit gets copied correctly in all corner-cases sounds like a major maintenance headache. If you wanna give this a shot nevertheless, you can use 6ca5f14 as a short-cut to come up with an alternative PR. |
|
It's not implicit to preserve the bits on the file you are copying. |
|
To flesh out the point about how surprising "just copy" can get: This is the state on main here Yet what we see in the bot repo (and I was able to reproduce locally) is IOW, conda-smithy/conda_smithy/feedstock_io.py Lines 132 to 138 in 3e8e768 silently ended up adding an executable bit, despite using the very appropriate-looking shutil.copymode.
|
|
Indeed. The doc string of copymode says it should copy the permissions bits. So that indicates a bug somewhere in the code that needs to be fixed. This PR is not fixing that bug, but is instead just papering over it with more code. Let's fix the bug. |
I'm awaiting your alternative implementation with interest. :) |
|
I think the bot has a different bug. It handles its own file permissions due to how containers are used. Smithy is doing the correct thing locally on my machine, and the block of code with I made a PR #2599 that puts in a smaller change just to Azure (where yaml files were incorrectly passed to the exe rendering function), and leaves the rest of the code for setting executable bits localized to the per-provider functions as it is now. |
The failure on the bot repo was on linux, and I reproduced it locally in WSL.
Are you looking at the file system (
I strongly doubt that this will fix anything, not least because the failures were not for azure-specific files, but for the |
|
The git staged thing shows the correct permissions on my machine. |
That's nice, but there's more machines involved. I can reproduce the error locally, so... Anyway, here's a branch with a self-contained commit (58aeb07) that you can pick anywhere and see if it works. On my machine, this fails with |
The failure didn't involve containers by the way; it was in the |
|
OK @h-vetinari. I made progress after dropping a kid at camp.
So IDK what is going on, but at least within smithy's own test suite, things are working fine. I am going to continue debugging the code in the feedstock OPs PR, since it appears to be the most direct way to trigger it. |
|
Ooooof. I found the actual bug, and it is not pretty. When conda-smithy 2026.6.14 was packaged in the feedstock, the permissions came out wrong on the files. 😱 Here is 2026.6.14 from the package on anaconda.org: (base) beckermr@finnegan conda-smithy-2026.6.14-unix_pyh9ac5cc3_0 % ls -lah site-packages/conda_smithy/feedstock_content/.ci_support/README
-rwxr-xr-x@ 1 beckermr staff 363B Jun 14 18:44 site-packages/conda_smithy/feedstock_content/.ci_support/READMEand here is 2026.5.29 from the package on anaconda.org: (base) beckermr@finnegan conda-smithy-2026.5.29-unix_pyh9ac5cc3_0 % ls -lah site-packages/conda_smithy/feedstock_content/.ci_support/README
-rw-r--r--@ 1 beckermr staff 357B May 29 15:24 site-packages/conda_smithy/feedstock_content/.ci_support/READMEI confirmed by testing conda-forge-feedstock-ops using a local install of smithy from source instead of the anaconda.org package in this CI run. Now the local rerender permissions come out right, but the ones in the container (which uses smithy from anaconda.org) come out wrong. So this PR is indeed papering over a more fundamental problem in the tooling somewhere. The next place to look is in the CI jobs for smithy 2026.6.14. |
|
Yup, there it is: from this CI job. Oh boy today is fun! |
|
Nope. Must be conda-build or something... |
|
debugging moved to here: conda-forge/conda-smithy-feedstock#394 |
|
Oh wow this is weird:
|
|
Finally found it. @h-vetinari, when you uploaded the source for smithy 2026.6.14 as a tar.gz to the release, the files had the wrong permissions: IDK how that happened on your end, but that is the cause of the permissions issues. Smithy itself appears to work fine. Any ideas on how that happened on your end? In any case, I am going to mark version 2026.6.14 as broken and try to upload a new version myself later today. This PR is not needed, as it is not fixing the actual bug. |
That's a question of point-of-view... I consider it as insulating us from the shenanigans that might be happening elsewhere, and putting the executable bits under our full control. Given all the bits and pieces involved where this could go wrong, this seems like the much simpler approach to me. |
Running rever in WSL, apparently 😭 (though this did work fine in the past) It highlights the main issue to me though - even if someone manages to change the permissions in smithy's contents (by accident or maliciously), we should copy defensively without the exec bits. It shuts off a whole class of attacks by design. |
|
Honestly dude, I'm done. I spent a ton of time debugging a CI issue that you incorrectly diagnosed, and yet you are still arguing with me that this PR should be merged. This pr tries to a fix a bug that doesn't exist. I literally added the test to another pr and it passed. If you want to debug something, go fix whatever it is in your machine that caused the bad release. From my perspective, it is nearly impossible to reason with you on a host of items. If you feel so strongly, then you can take ownership of it all, bot included. |
|
Let's take this discussion somewhere where we can unjinx this. I wrote you on Zulip. |
To illustrate what I meant with the comment you're apparently reacting to, I've reverted out the one thing you consider pointless/wrong (which we can disagree on, fine); the rest is still a clear improvement though - not sure why it's surprising that I'm arguing we should keep this? Beyond the cold facts, apologies for exasperating you, and thanks for the time you invested. I also spent a lot of time on this yesterday; obviously I didn't choose for the release workflow to break in creative new ways. Let's address the rest of your message in a private discussion. |
|
No thank you. I appreciate the revert, but some things just can't be undone that easily. |
What in the world are you talking about? I'd be willing to hash this out, but I don't even know what "things that can't be undone that easily" I've apparently committed in your eyes - unwittingly, because I have no reason or interest to offend you. That's why communication would be important; to my mind, we've worked well together for years - so it's hard for me to fathom why we couldn't solve some perturbations in our interaction, or indeed give some benefit of the doubt to the other. |
|
I should clarify that my comment was about my decision regarding my own boundaries, not about anything very specific to this PR. The reality for me is that I cannot constantly feel like I am going uphill when working on PRs, debugging, etc., with you. It's been a battle at times for me to keep my cool for a long time, and now it's spilling over. That's on me for sure, but the fact remains that it's just far too difficult for us to find agreement on basic facts (e.g., "there isn't a bug since the test that is supposed to fail if the bug is there is passing" or "one piece of code cannot possibly be responsible for correctly parsing arbitrary output from another"). It's causing us to spend effort on tiny details instead of the big picture (e.g., "smithy needs a new release process" or "we can't roll updates to codes that have no API promises into our infrastructure on-the-fly without testing"). I don't want to engage in that anymore because it is frustrating, time-consuming, and not rewarding. I do respect your time and effort, but whatever is happening here isn't working. |
This is the first time I'm hearing of this from you. I'm sorry about that, and I'm sure it's something that can be fixed, if we make an effort to talk about it.
Both of these are not things I said; though I see how you might have inferred them, it's hard to take responsibility for things I didn't say. I fully agree on the overall direction (fixing things, keeping things running smoothly, updating/improving the release process, etc.), so... I really don't see why any disagreement couldn't be resolved, and easily at that. In this case my impulse would be to completely rule out a class of errors, rather than chase whatever piece of the rube goldberg machine may be responsible. That's not out of laziness to fix things, but because I consider it the more stable & controlled long-term solution. That doesn't mean though that it's the only way to fix it; you didn't like my solution, and once you found the culprit, I was like "ok fine, but let's keep the other improvements at least". At no point was this about anything personal, and honestly, I take offence at "impossible to reason with". If you (or anyone else for that matter) makes an argument, I'm willing to take it on its merits, which may certainly trump my arguments. If you feel differently, please show me an example where that wasn't the case. |
|
My apologies @h-vetinari for my comments on this thread. I unloaded a fair bit of pent up frustration over our interactions that should have been expressed in a more collegial tone. The frustration I have is real to me and is that I do want to feel that things which I think matter are taken seriously by you. I don't always feel that way. |
|
Thank you, apology accepted.
I'm sorry to hear that. On my end, you did not voice this to me, much less give me an opportunity to respond or change something. I'm hard-pressed to remember anything infrastructure-related I didn't take seriously1, so this sounds like a complete misunderstanding to me? I do feel quite strongly that after many years of productive/pleasant collaboration, I would have deserved a more charitable interpretation of my actions/statements, and a calm opportunity (through whatever channel) to address what you perceived I wasn't taking seriously. This reminds me of a dictum that might be useful here, if only for finding a path back towards levity: when you ASSUME, you make an ASS out of U and ME. 😉 Footnotes
|
2 participants
As it turns out, there are circumstances where conda-smithy will wrongly populate a rerendered recipe with executable bits also for non-executable files, e.g.
which is what we ran into with conda-forge/conda-forge-bot#6237.
In fact, we're definitely generating executable bits on
.azure-pipelines/*.yamls, e.g. on the current main branch of arrow-cpp-feedstock (note the 755 at the beginning):due to the fact that those files are treated as templates
conda-smithy/conda_smithy/configure_feedstock.py
Lines 1990 to 2004 in 3e8e768
which is borked, because
conda-smithy/conda_smithy/configure_feedstock.py
Line 1713 in 3e8e768
just sets the executable bit unconditionally for everything.
So to clean this up, copy everything as non-executable by default (9457bb2), then distinguish executable files from templates, and add the exec bit only for the former. Add a test that validates that all executable files have the exec bit, and that all other files don't.