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

Extend and improve acre #2

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Extend and improve acre #2

wants to merge 19 commits into from

Conversation

aardschok
Copy link
Collaborator

Part of issue FUS-32

  • Added launcher module
  • Added which and execute function to core
  • Retrieve PATH and PATHEXT from env in which, fallback to os.getenv if PATHEXT is not available

return None


def execute(executable, args=None, environment=None, cwd=None):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for refactoring this to execute. It's the same method as in Avalon pipeline currently, but there it's called launch? Any reason why the original was a bad name?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aardschok should we still refactor this to launch?

@aardschok aardschok changed the title Extend and improve env prototype Extend and improve acre Apr 10, 2018
@BigRoy
Copy link
Owner

BigRoy commented Apr 11, 2018

Still not too sure about having the execute and which in acre - because of the added complexity. Maybe we should pull this to a branch instead of the master, to visualize it isn't the live master? Or, since it hasn't been adopted by others yet (as far as I can tell) maybe just merge this but then get the cleaned up API out in a day or two.

@aardschok
Copy link
Collaborator Author

I think we can rename which to locate as that is exactly what the function docs say. This way we make it clear it locates the executable in the current environment.

@aardschok
Copy link
Collaborator Author

Tested it for Maya and Fusion deadline submission, had to add avalon-setup/bin/pythonpath and avalon-setup/git/pyblish-base to the environment to ensure the Avalon initialization within Maya worked properly. This might be a subject for a discussion.

@aardschok
Copy link
Collaborator Author

I do not think that we should care about familiarity of others when using this. We could have chosen where for all that matters because we are on Windows. If anything, locate is both which and where.

Let's tackle it from a pure logical standpoint without taking in account the OS in which it is used.

  • which: this points more to the name of an object
  • where: there points to the location of the object.

Now based on pure grammar: we already KNOW the object, we pass it on to the function. Taking that logic in account both where and locate make more sense because we are looking for the given object.

I am entirely in favor to use locate because which is not a standard and sounds illogical.

@BigRoy
Copy link
Owner

BigRoy commented Jul 5, 2018

No, where on Windows has a different behavior - it's not the same. :) The function that's here in our code is exactly the functionality of which on UNIX.

The name which comes from the fact that the same executable name can be on your path, e.g. python.exe in Python3 and Python2. You use which python to find which of the two is being used. It's not locating both for you.

Again, subtle difference. I'm fine with locate if you like it much more. I just wanted to address that if you use which you'll have the added benefit of those on UNIX knowing the command since it's exactly that. :)

@aardschok
Copy link
Collaborator Author

Any objection to merging this?

acre/launch.py Outdated
import os
import pprint

from . import discover, build, merge, locate, launch
Copy link
Owner

@BigRoy BigRoy Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this importing itself? Isn't this a naming conflict? :) Interesting that this is actually allowed.

@BigRoy
Copy link
Owner

BigRoy commented Jul 5, 2018

Other than above note this is fine for me - it's a prototype. Like mentioned in our discussions I think the API can still simplify by just having the locate, build and merge

By the way, don't you think locate and discover sounds so very similar? When I saw the import statements I suddenly thought: "what's the difference?"

@aardschok
Copy link
Collaborator Author

Made some changes to the API, I would like to have a review on the prepare

I think that there is still some room for improvements

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

Successfully merging this pull request may close these issues.

2 participants