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

Use bash calls explicitly instead of relying on executable permissions #330

Closed
nicoddemus opened this issue Oct 17, 2016 · 15 comments
Closed
Labels

Comments

@nicoddemus
Copy link
Member

As discussed in conda-forge/pytest-cov-feedstock#6, conda smithy should use explicit bash calls instead of relying on executable permissions because those can get lost on Windows.

In that specific case, I was running a vagrant box on Windows and cloned pytest-cov-feedstock in a shared folder in order to re-render it.

@jakirkham
Copy link
Member

If you have some time, I'd be very curious to know what happens when you do the following.

  1. Clone the libgfortran feedstock.
  2. Change the skip to be Windows only.
  3. Re-render with conda-smithy
  4. Inspect the permissions of ci_support/run_docker_build.sh and ci_support/checkout_merge_commit.sh.

If they differ, this would be very interesting to know. If they are the same, this would also be good to know (though less surprising).

@nicoddemus
Copy link
Member Author

Done. I cloned the libgfortran on my shared folder (/vagrant).

diff --git a/recipe/meta.yaml b/recipe/meta.yaml
index b2f928d..8e43c49 100644
--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -9,7 +9,7 @@ package:

 build:
   number: 0
-  skip: true                                                  # [not osx]
+  skip: true                                                  # [win]
   always_include_files:
     - lib/libgfortran.dylib                                   # [osx]
     - lib/libgfortran.{{ libgfortran_version[0] }}.dylib      # [osx]
vagrant@thdengops-ubuntu-14:/vagrant/libgfortran-feedstock$ ls -la ci_support/
total 7
drwxrwxrwx 1 vagrant vagrant    0 Oct 17 21:57 .
drwxrwxrwx 1 vagrant vagrant 4096 Oct 17 21:57 ..
-rwxrwxrwx 1 vagrant vagrant  649 Oct 15 19:10 checkout_merge_commit.sh
-rwxrwxrwx 1 vagrant vagrant 1366 Oct 17 21:57 run_docker_build.sh

Please let me know if you would like me to do some other tests.

@jakirkham
Copy link
Member

Ok, so that isn't surprising. Were you adding and committing the files with git on the Windows side or the Linux side before (guessing Windows)? I'm trying to verify what the result of that step would be (guessing neither has executable permissions).

@nicoddemus
Copy link
Member Author

Were you adding and committing the files with git on the Windows side or the Linux side before (guessing Windows)?

On Linux.

@jakirkham
Copy link
Member

Ah right because you said it was a shared folder. So the executable permissions don't get copied over to the host right?

@jakirkham
Copy link
Member

In any event, the simple fix should be PR ( #332 ). This should ensure everything runs through bash. Please let me know if you notice anything else like this that we need to fix and we can tack it on in that PR.

@nicoddemus
Copy link
Member Author

Thanks for the quick fix! 😄

@jakirkham
Copy link
Member

As another option to consider (that has few other bonuses), have put together PR ( #336 ). It will automatically stage all changes in the index (assuming we have a git repo) including permission changes. As a bonus, this PR provides an option to commit either with the editor popped up or automatically (batch mode).

Would be curious to see if this works well for you on Windows or not. Even if it doesn't help this issue per se, it will at least make doing lots of re-renderings much easier. 😄

@nicoddemus
Copy link
Member Author

As another option to consider (that has few other bonuses), have put together PR ( #336 ).

Thanks for the pointer, I left a comment on that PR. 😁

Would be curious to see if this works well for you on Windows or not. Even if it doesn't help this issue per se, it will at least make doing lots of re-renderings much easier.

Agreed, looks like a nice addition regardless. Btw, the last time I tried using conda smithy on Windows natively it didn't really work, that's why I was using Vagrant to perform the rerendering. Is conda smithy supposed to work on Windows? If so I can try again and report the problems I find.

@jakirkham
Copy link
Member

Honestly I'd love to get a Windows user's feedback on conda-smithy. However, as you note, we have a big blocker ATM due to the msys2 channel configuration. ( #281 ) Once that is addressed, we can talk more about Windows testing and hopefully you won't need to use a VM any more. 😄

@nicoddemus
Copy link
Member Author

Once that is addressed, we can talk more about Windows testing and hopefully you won't need to use a VM any more.

Cool, I would be glad to help test it. I've subscribed to that issue to follow it closely.

Thanks again for the incredible environment and excellent support!

@jakirkham
Copy link
Member

jakirkham commented Oct 29, 2016

So I'm releasing 1.4.2, which contains the git improvements. I would be curious to know if that solves the problem for your use case or not. If it does solve it, I'd be inclined to leave the circle.yml file untouched.

Edit: Made a few minor patch releases since that should improve the user experience.

@nicoddemus
Copy link
Member Author

@jakirkham thanks! I will try it again the next time I have to re-render something, I will be sure to give you some feedback then.

Thanks again for the awesome support!

@jakirkham
Copy link
Member

So it looks like we can say tentatively that PR ( #336 ) fixed this based on field testing. Should we close this out or would you like to do some more testing to make sure?

@nicoddemus
Copy link
Member Author

I'm good! Thanks again! 😄

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

No branches or pull requests

2 participants