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

Should arcade be invoking chmod on a checked in file? #12957

Open
joperezr opened this issue Mar 24, 2023 · 5 comments
Open

Should arcade be invoking chmod on a checked in file? #12957

joperezr opened this issue Mar 24, 2023 · 5 comments

Comments

@joperezr
Copy link
Member

I'm trying to setup a .devcontainer for a new repo that uses arcade in order to onboard to GitHub Codespaces. While doing so using VS Code Dev Container tooling, I'm hitting a permission error during the prebuild given that arcade is trying to invoke chmod command on the checked-in script eng\common\dotnet-install.sh before it invokes it. That happens on this line:

<Exec Condition="'$(OS)' != 'Windows_NT'"
Command="chmod +x &quot;$(_DotNetInstallScript)&quot;" />

The error is most likely because of the limited permissions that the user that VSCode uses has, so it can't perform chmod, but I'm actually curious as to why would arcade even have to do this to begin with. Given this is a checked in file, this shouldn't be a problem as git should be used in the first place to ensure the file is added with the right index and permissions so it can be executed and this command be avoided. That said, I'm not very familiar to what this targets file is used for so I would like to understand what it is trying to do. I did a quick blame on the file and looks like those lines where added by @natemcmaster 4 years ago, so I'm wondering if they are still needed.

cc: @eerhardt

@garath
Copy link
Member

garath commented Jun 2, 2023

This seems like a good question. I wonder if there is some difficulty keeping that bit set since Windows is so much in the mix.

@garath
Copy link
Member

garath commented Jun 2, 2023

The original issue for that change was #2725. It seems like storing the file as executable may have just been overlooked or unknown.

Anyone want to put together a PR to test?

@missymessa
Copy link
Member

@dougbu to follow up on this one

@dougbu
Copy link
Member

dougbu commented Jun 15, 2023

Note, the bits in GitHub are relatively simple to handle — even on a Windows machine:

  1. git update-index --chmod=+x path/to/file to add the bit
  2. git update-index --chmod=-x path/to/file to remove it
  3. git ls-files --stage {files} e.g., image

The specific file mention in the original issue is set correctly (755 as shown above) already. I think the following shows the same thing in the GitHub UI:
image

All that said, if the code above still exists for a checked-in file, we should clean it up. @missymessa does this complete my part❔

@garath
Copy link
Member

garath commented Jun 29, 2023

does this complete my part❔

Thank you!

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

No branches or pull requests

4 participants