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

blender: fix wrapper script on case-sensitive filesystems #21990

Merged
merged 1 commit into from
Jun 16, 2016

Conversation

psibre
Copy link
Contributor

@psibre psibre commented Jun 15, 2016

fixes a regression introduced in 7aabd17

@fanquake fanquake added the awaiting maintainer feedback Issue needs response from a maintainer. label Jun 15, 2016
@vitorgalvao vitorgalvao self-assigned this Jun 15, 2016
@vitorgalvao
Copy link
Member

Is this still needed, now that we move to /Applications?

@vitorgalvao vitorgalvao added awaiting user reply Issue needs response from a user. and removed awaiting maintainer feedback Issue needs response from a maintainer. labels Jun 15, 2016
@jawshooah
Copy link
Contributor

The shim was added in #18891 to provide a custom PYTHONPATH, so I doubt we can get rid of it.

@vitorgalvao
Copy link
Member

The shim was added in #18891 to provide a custom PYTHONPATH, so I doubt we can get rid of it.

I’m asking because in a recent issue (which I’m not being able to find), there was talk of Blender making this wrapper itself, but since it relied on hardcoded paths, this shim was needed instead. With moving, that would presumably be fixed. I think I even pinged someone (likely @psibre) but did not get an answer.

@psibre
Copy link
Contributor Author

psibre commented Jun 16, 2016

Thanks for the feedback! It does appear that in the meantime, the shim script is no longer necessary to run blender with various arguments (such as for batch rendering with a custom python script)... I've tried just running /Applications/Blender.app/Contents/MacOS/blender in this way, and it seems to be working. I can push another commit that drops the shim script and links the binary directly.

The only reservation I have is that I haven't tested this in a sandbox environment, and it's not unlikely that I have local configurations that somehow make this work on my machine for other reasons.
This brings back the question of functionally testing a cask, perhaps with a postflight block, to ensure that the everything works as expected... Is this a thing now?

@vitorgalvao
Copy link
Member

This brings back the question of functionally testing a cask (…) Is this a thing now?

It’s not. And since many of our CLI tools will be removed to be in homebrew instead, there’s even less of a reason.

It does appear that in the meantime, the shim script is no longer necessary to run blender with various arguments

Let’s drop the shim, then. I’d rather see not having it break later on and have to revert, than have it there and not be needed.

@psibre
Copy link
Contributor Author

psibre commented Jun 16, 2016

@vitorgalvao I would absolutely agree.

However I just ran an informal test, creating a small two-liner for blender:

import bpy
print(bpy.app.version_string)

Running this with the fixed shim script in this PR:

$ blender --background --python ~/bpytest.py 
found bundled python: /Applications/Blender.app/Contents/MacOS/../Resources/2.77/python
2.77 (sub 0)

Blender quit

Then I removed the shim script from the cask:

 Casks/blender.rb | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/Casks/blender.rb b/Casks/blender.rb
index 0e8eee1..d0ea3c5 100644
--- a/Casks/blender.rb
+++ b/Casks/blender.rb
@@ -10,17 +10,5 @@ cask 'blender' do
   # Renamed for consistency: app name is different in the Finder and in a shell.
   app 'blender.app', target: 'Blender.app'
   app 'blenderplayer.app', target: 'Blenderplayer.app'
-  # shim script (https://github.com/caskroom/homebrew-cask/issues/18809)
-  shimscript = "#{staged_path}/blenderwrapper"
-  binary shimscript, target: 'blender'
-
-  preflight do
-    pythonversion = '3.4'
-    File.open(shimscript, 'w') do |f|
-      f.puts '#!/bin/bash'
-      f.puts "export PYTHONHOME=#{Hbc.appdir}/Blender.app/Contents/Resources/#{version}/python/lib/python#{pythonversion}"
-      f.puts "#{Hbc.appdir}/Blender.app/Contents/MacOS/blender $@"
-      FileUtils.chmod '+x', f
-    end
-  end
+  binary "#{Hbc.appdir}/Blender.app/Contents/MacOS/blender"
 end

Upon which, sadly, running the test script fails:

$ blender --background --python ~/bpytest.py 
Color management: using fallback mode for management
BLT_lang_init: 'locale' data path for translations not found, continuing
Warning! bundled python not found and is expected on this platform. (if you built with CMake: 'install' target may have not been built)
Fatal Python error: Py_Initialize: unable to load the file system codec
ImportError: No module named 'encodings'

Current thread 0x00007fff75951180 (most recent call first):
Illegal instruction: 4

Followed by the usual OSX crash popup window.

So it looks like we still need the shim script after all, despite my earlier comments, and the optimistic documentation upstream...

@vitorgalvao
Copy link
Member

Alright, seems like we have to keep it, then. Thank you for looking further into it.

Have you contacted upstream about the issue?

@vitorgalvao vitorgalvao merged commit c3da086 into Homebrew:master Jun 16, 2016
@psibre
Copy link
Contributor Author

psibre commented Jun 16, 2016

@vitorgalvao I'm not sure if this really is the correct way to run blender through CLI on OSX. It works for me, hopefully for everyone else as well, and you guys are super-active and dynamic in maintaining HBC, so I'm happy.
I could certainly report upstream that the "optimistic documentation" is not quite correct, point to this working shim, and see what happens.

@psibre psibre deleted the blender branch June 16, 2016 21:06
@vitorgalvao
Copy link
Member

I could certainly report upstream that the "optimistic documentation" is not quite correct, point to this working shim, and see what happens.

That’s what I mean. Informing them their method doesn’t always work, and why, so they can fix it upstream (in which case everyone benefits, not just HBC users).

@adidalal adidalal removed the awaiting user reply Issue needs response from a user. label Jul 2, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants