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

Fix forwarding script on Windows (suite supporting) #968

Merged

Conversation

davidlatwe
Copy link
Contributor

@davidlatwe davidlatwe commented Oct 27, 2020

Problem

The forwarding script is basically an executable yaml file that has shebang which made it can be executed by _rez_fwd.

But this doesn't work on Windows, which means that tools in rez-suite cannot be found nor executed, and the build script that generated by rez-build --scripts won't work either.

Solution

Adding extension .cmd to the forwarding script, makes it a semi-batch-script, e.g.

@echo off
_rez_fwd.exe %~dpnx0
goto :eof
:: YAML
func_name: _FWD__spawn_build_shell
kwargs:
    ...

The line goto :eof will skip the YAML content when this script is being called in terminal, and the first 4 cmd script lines will be stripped before parsing into dict by yaml.load in forward.py.

I also fixed the missing import statement in custom build system on my way out.

@davidlatwe davidlatwe changed the title Fix forwarding script on Windows Fix forwarding script on Windows (suite supporting) Oct 28, 2020
@davidlatwe
Copy link
Contributor Author

Sorry for the force push, was trying to get rid of commits which turns out to be bad approach in test.

Mainly because Rez is not activated (rez bin is not in PATH) in the CI test, and the suite tools require _rez_fwd to work or test fail. But if I change the Action workflow yaml to add Rez in PATH, Windows docker image will need to be rebuilt by @nerdvegas which may cause too much trouble. Anyway, based on the README in rez.test dir, not activating Rez seems to be deliberately, so I changed to inject rez bin path solely in that test case.

Now, with that test case, should be enough to prove rez-suite can work cross platform. 🤔

Copy link
Contributor Author

@davidlatwe davidlatwe left a comment

Choose a reason for hiding this comment

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

Has anyone tried this on Windows ? 🤓

# activate suite
env["PATH"] = os.pathsep.join([bin_path, env["PATH"]])

output = subprocess.check_output(["hunny"], shell=True, env=env,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have change this to use tool's context.execute_shell(block=True), then I won't have to put rez_bin_path manually. Didn't know that before.

Will fix that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, scratch that. The purpose of this test case is to test suite tool can be executed from a rez activated shell, not a resolved context.
Doc string added in f4ca471 to be more clear.

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