Fixes some failing tests on windows #775

willjp · 2019-10-25T15:50:55Z

I took a stab at addressing some of the failing tests under python3+windows.

corrects failing tests in rez/tests/test_packages.py on windows
adds %SYSTEMROOT% envvar to Python(ActionInterpreter) so rez/tests/test_context.py tests pass on windows

filesystem.FileSystemPackageRepository.location is lowercased on platforms with case-insensitive filesystems. Note that these test failures did *not* appear when using `pytest` as a testrunner.

…does not fail on windows. python -m unittest src/rez/test_context.py test ``test_execute_command_environ`` failed under windows with python3.6. Subprocess failed printing the following to stderr: Fatal Python Error: failed to get random numbers to initialize Python Problem only appears *if* %PATH% is configured with python on it, but without %SYSTEMROOT%. example: # Works Popen(['python', '-V']) # Works Popen(['python', '-V'], env={'PATH': 'C:\\Python-3.6.5', 'SYSTEMROOT': 'C:\Windows'}) # Fails Popen(['python', '-V'], env={'PATH': 'C:\\Python-3.6.5'})

…fixes

bfloch · 2019-10-25T16:03:16Z

Haven't tried your PR yet, but I think version and changelog are handled by @nerdvegas

willjp · 2019-10-25T16:30:58Z

oh! Sorry, I was trying to follow the directions in CONTRIBUTING.md . I can undo the commit though if it is a problem.

bfloch · 2019-10-25T16:38:46Z

I think you might be right. It's just that this was recently updated and I misunderstood the change:
https://github.com/nerdvegas/rez/blob/master/RELEASE.md

bfloch · 2019-10-25T16:41:39Z

Do you think you change can in future be represented by the mechanism proposed here:
#737

Not saying that we don't need the workaround for the time being, but just checking if the proposal
might miss a feature.

willjp · 2019-10-25T17:12:08Z

Oh that looks very nice! No sense hiding conditionals all over the place :)

If I have understood your proposal correctly:

The first commit's fixes would be addressed by your "modify_op": PlatformDependent({"windows": "windowspath"}) to format the path consistently. It would just need to be used on both ends, or have a special object that can be used to test equality.
The second commit is kind of weird, and specific python-3 on windows. I'm curious if it is even needed in later versions of python. A cleaner workaround within your proposal might define a whitelist of variables to inherit from the parent process unless they are assigned by the user?

Sorry, I got excited and wanted that green tests-passing badge across the board :)

bfloch · 2019-10-25T19:03:51Z

Me, too. Other then your fix I am working towards fixing the other Windows tests in #773

nerdvegas · 2019-10-25T20:36:43Z

Just two general points: 1. Yes the contributing guidelines were changed yesterday, but I only mentioned on slack, nw. 2. Generally if we need to put in a fix that we also know will change at some point in future (eg as Blazej mentions about #737), I'm fine with that, but please please put a big fat comment there highlighting the fact. Cheers! A

…

On Sat, Oct 26, 2019 at 6:03 AM Blazej Floch ***@***.***> wrote: Me, too. Other then your fix I am working towards fixing the other Windows tests in #773 <#773> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOUSVPP7AICZATAVSWZ73QQM7JTANCNFSM4JFFBTQQ> .

nerdvegas · 2019-10-25T20:57:58Z

src/rez/rex.py

@@ -633,6 +634,7 @@ def error(self, value):
    def subprocess(self, args, **subproc_kwargs):
        if self.manager:
            self.target_environ.update(self.manager.environ)
+        self.target_environ = self.adjust_env_for_platform(self.target_environ)


I would drop the return value, and avoid the unnecessary dict copy. adjust_env_for_platform should just alter self.target_environ in-place, asthe previous code already does

nerdvegas · 2019-10-25T20:58:21Z

src/rez/rex.py

+        """ Make required platform-specific adjustments to env.
+        """
+        if sys.platform.startswith('win'):
+            env = self._add_systemroot_to_env_win32(env)


Same as above, in-place rather than copy

nerdvegas · 2019-10-25T20:58:48Z

src/rez/rex.py

+
+        """
+        # 'SYSTEMROOT' only relevant on windows
+        if not sys.platform.startswith('win'):


this check is already done, and this is a non-public func, so this can be removed

fixed!

thank you, I'm never sure how much I should worry about accidents. this seems like a good rule of thumb.

nerdvegas · 2019-10-25T21:00:29Z

src/rez/rex.py

+        if 'SYSTEMROOT' not in os.environ:
+            return env
+
+        new_env = env.copy()


as above, in-place rather than copy

nerdvegas · 2019-10-25T21:09:58Z

src/rez/tests/test_packages.py

+        # on case-insensitive platforms (eg windows)
+        base = os.path.join(self.py_packages_path, "variants_py", "2.0")
+        if not platform_.has_case_sensitive_filesystem:
+            base = base.lower()


should move _format_platformpath and reuse here instead

Actually, this sort of code already exists in another place, see https://github.com/nerdvegas/rez/blob/master/src/rezplugins/package_repository/filesystem.py#L473.

It would be good to add a utility function to rez.utils.filesystem, something like so:

def canonical_path(path, platform=None): if platform is None: from rez.utils.platform_ import platform_ platform = platform_ path = os.path.realpath(path) if not platform.has_case_sensitive_filesystem: path = path.lower() return path

I think this may also fix a potential bug currently, where two package repositories could be the same, but if one uses symlinks to the same path on disk, rez will think they are two different repos.

To give some context: The lower() is there to fix a previous bug that was caused by differing cases of the same path, on windows (non-case-sensitive), which then led rez to believe that there were two different packages that were in fact the same package ('foo' vs 'Foo' for eg).

At least if there's just the one utility function, this can be documented in one place.

Thank you, I agree with this idea.

The FileSystemPackageRepository that you provided a link to is where I copied the logic from, and was the source of the test-failure under windows.

I wrote canonical_path in addition to tests for it.

There are some frustrating pain points here for testing however.

# python-3.6.5, windows-10 ntpath.realpath('/a/b/c') #>> 'C:\\a\\b\\c' posixpath.realpath('C:/a/b/c') #>> 'C:\\Users\\willjp/C:/a/b/c' # python-3.7.4, windows-10 ntpath.realpath('/a/b/c') #>> 'C:\\a\\b\\c' posixpath.realpath('C:/a/b/c') #>> 'C:\\Users\willjp/C:/a/b/c'

# python-3.7.4, linux ntpath.realpath('/a/b/c') #>> '\\a\\b\\c' posixpath.realpath('/a/b/c') #>> '/a/b/c'

This change in python is not represented in the docs, so I am uncertain when they were introduced: https://docs.python.org/3/library/os.path.html?highlight=realpath#os.path.realpath

willjp · 2019-10-26T14:29:48Z

Thank you very much for the code review!

I'm updating the code now, and I'll post again once I hve it fixed.

and handles windows directory-separators (`\\`).

…path`

…ing a copy.

willjp · 2019-10-26T16:56:50Z

Firstly, thanks very much for the code-review! I really appreciate the help.

I updated the code:

new rez.utils.filesystem.canonical_path()
new test file rez.tests.test_utils.test_filesystem to test canonical_path
rez.tests.test_context uses canonical_path
rezplugins.package_repository.filesystem.FileSystemPackageRepository uses canonical_path

Unrelated to previously mentioned:

rez.utils.filesystem.to_nativepath() was broken (os.path.join requires an expanded list). I also adjusted it to use replace('\\', '/') .

Misc Questions:

I noticed from old imports that you previously had a rez.vendor.mock but removed it. None of the other tests use mocks, so I'm guessing that you made an intentional decision to use TestCase.skipTest() instead. I followed this pattern, but just wanted to confirm this is how I should be proceeding.
Out of curiosity, what is the goal ofrez.utils.filesystem.{to_nativepath, to_ntpath, to_posixpath} versus {os.path, ntpath, posixpath}.normpath ? I feel like the latter might accomplish more, but I didn't want to break anything you already have in place.

Finally:

Thanks for clearing up about CONTRIBUTING.md - In the future, I'll leave the version and changes files alone. :)

bfloch · 2019-10-28T21:57:02Z

Together with this PR I get green tests on Windows via docker for all three python version.

nerdvegas · 2019-10-28T22:10:58Z

Nice, will get this merged soon. I've asked ASWF about their container hosting, haven't heard back yet. A

…

On Tue, Oct 29, 2019 at 8:57 AM Blazej Floch ***@***.***> wrote: Together with this PR I get green tests on Windows via docker for all three python version. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOUSUJQBLG6R5I2263AGDQQ5N3BANCNFSM4JFFBTQQ> .

JeanChristopheMorinPerso · 2019-10-31T02:30:40Z

@willjp For the test file you added, instead of rez/tests/test_utils/test_filesystem.py, we will need a rez/tests/test_utils_filesystem.py. This is so that the test can be shown when using rez-selftest.

Adds header/footer so more consistent with other tests.

willjp · 2019-10-31T15:13:41Z

sure, fixed!

Actually while on the subject of tests - do you want this testfile? None of the other utils are tested directly, I wasn't sure if this was something you guys would want, or if this was unecessary.

JeanChristopheMorinPerso · 2019-10-31T15:19:09Z

Tests are always welcome. We are lagging behind in terms of tests, so yes multiple parts of rez are not really tested. In general, I think we have about 43% of coverage. Thanks for the changes and adding the new tests :)

willjp · 2019-10-31T15:39:17Z

No problem! And that's good to know, I'll write tests as I go then :)

willjp · 2019-10-31T15:42:20Z

I'm failing on MacOS, I'll fix this

Addresses failing MacOS tests - `/var/tmp` is symlinked to `/private/var/tmp` on MacOS

with python version.

nerdvegas · 2019-11-01T23:17:54Z

Out of curiosity, what is the goal ofrez.utils.filesystem.{to_nativepath, to_ntpath, to_posixpath} versus {os.path, ntpath, posixpath}.normpath ? I feel like the latter might accomplish more, but I didn't want to break anything you already have in place.

Good question. I don't remember, it's worht investigating. Could be that py2.6 didn't have that support? Not sure.

Made a ticket: #783

bfloch · 2019-11-05T21:50:21Z

@nerdvegas @willjp Wondering if we could make the test_utils_filesystem.py -> test_utils.py for now.
I have literally a single test from data_utils (HashableDict) and I did not want to open a new test suite for it.
Since you also have a couple of tests I wonder if we could share that namespace and rather diverge in future when we have more tests.

Thoughts?

nerdvegas · 2019-11-05T22:08:49Z

Yeah I don't think it's unreasonable to have a general utils test, so go ahead. Rule here should just be that stuff tested in utils is low level and standalone. Cheers A

…

On Wed, Nov 6, 2019 at 8:50 AM Blazej Floch ***@***.***> wrote: @nerdvegas <https://github.com/nerdvegas> @willjp <https://github.com/willjp> Wondering if we could make the test_utils_filesystem.py -> test_utils.py for now. I have literally a single test from data_utils (HashableDict) and I did not want to open a new test suite for it. Since you also have a couple of tests I wonder if we could share that namespace and rather diverge in future when we have more tests. Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOUSSRF6BLOILTJSWCC23QSHTCBANCNFSM4JFFBTQQ> .

willjp · 2019-11-06T00:23:22Z

yeah absolutely, that sounds good to me. Give me a few minutes to finish up at work and I'll push an update.

willjp added 4 commits October 25, 2019 08:30

fixes windows test failures.

1a90a60

filesystem.FileSystemPackageRepository.location is lowercased on platforms with case-insensitive filesystems. Note that these test failures did *not* appear when using `pytest` as a testrunner.

Merge remote-tracking branch 'upstream/master' into rez_test_windows_…

64e6207

…fixes

updates version and changelog

a407270

Corrects logic. env==None is acceptable.

119d801

nerdvegas reviewed Oct 25, 2019

View reviewed changes

willjp added 5 commits October 26, 2019 11:32

Adds canonical_path() to rez.utils.filesystem , alongside new tests.

ddab4c7

correction to rez.utils.filesystem.to_nativepath() so that it runs,

6e9b385

and handles windows directory-separators (`\\`).

replaces _format_platformpath with `rez.utils.filesystem.canonical_…

5fe1ab4

…path`

self.target_environ assigned directly rather than modifiying/return…

e39cf7e

…ing a copy.

replaces conditional with canonical_path()

f8f6666

PEP8 fixes. sorry

6cdf914

bfloch mentioned this pull request Oct 30, 2019

Windows Tests via Docker #781

Merged

moves test_utils/test_filesystem -> test_utils_filesystem.

cab6b8d

Adds header/footer so more consistent with other tests.

bfloch mentioned this pull request Oct 31, 2019

Adds PlatformDependent factory. #739

Open

willjp added 2 commits October 31, 2019 19:11

replaces testpath /var/tmp with a contrived path.

3c020f5

Addresses failing MacOS tests - `/var/tmp` is symlinked to `/private/var/tmp` on MacOS

skip message is more explicit. realpath() behaviour has nothing to do

7022990

with python version.

renames test_utils_filesystem -> test_utils

fa24878

nerdvegas merged commit fa24878 into AcademySoftwareFoundation:master Nov 6, 2019

willjp deleted the rez_test_windows_fixes branch November 6, 2019 23:07

jimporter mentioned this pull request Feb 22, 2020

Python error: failed to get random numbers appveyor/ci#1995

Closed

JeanChristopheMorinPerso mentioned this pull request Mar 16, 2023

Environment resolution uses UNC paths with Python 3.10, 3.7 was using mounted network drives. #1438

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes some failing tests on windows #775

Fixes some failing tests on windows #775

willjp commented Oct 25, 2019

bfloch commented Oct 25, 2019

willjp commented Oct 25, 2019

bfloch commented Oct 25, 2019

bfloch commented Oct 25, 2019

willjp commented Oct 25, 2019

bfloch commented Oct 25, 2019

nerdvegas commented Oct 25, 2019 via email

nerdvegas Oct 25, 2019

willjp Oct 26, 2019

nerdvegas Oct 25, 2019

willjp Oct 26, 2019

nerdvegas Oct 25, 2019

willjp Oct 26, 2019

nerdvegas Oct 25, 2019

willjp Oct 26, 2019

nerdvegas Oct 25, 2019

nerdvegas Oct 25, 2019

willjp Oct 26, 2019 •

edited

Loading

willjp commented Oct 26, 2019

willjp commented Oct 26, 2019

bfloch commented Oct 28, 2019

nerdvegas commented Oct 28, 2019 via email

JeanChristopheMorinPerso commented Oct 31, 2019

willjp commented Oct 31, 2019

JeanChristopheMorinPerso commented Oct 31, 2019

willjp commented Oct 31, 2019

willjp commented Oct 31, 2019

nerdvegas commented Nov 1, 2019

bfloch commented Nov 5, 2019

nerdvegas commented Nov 5, 2019 via email

willjp commented Nov 6, 2019

Fixes some failing tests on windows #775

Fixes some failing tests on windows #775

Conversation

willjp commented Oct 25, 2019

bfloch commented Oct 25, 2019

willjp commented Oct 25, 2019

bfloch commented Oct 25, 2019

bfloch commented Oct 25, 2019

willjp commented Oct 25, 2019

bfloch commented Oct 25, 2019

nerdvegas commented Oct 25, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willjp Oct 26, 2019 • edited Loading

Choose a reason for hiding this comment

willjp commented Oct 26, 2019

willjp commented Oct 26, 2019

bfloch commented Oct 28, 2019

nerdvegas commented Oct 28, 2019 via email

JeanChristopheMorinPerso commented Oct 31, 2019

willjp commented Oct 31, 2019

JeanChristopheMorinPerso commented Oct 31, 2019

willjp commented Oct 31, 2019

willjp commented Oct 31, 2019

nerdvegas commented Nov 1, 2019

bfloch commented Nov 5, 2019

nerdvegas commented Nov 5, 2019 via email

willjp commented Nov 6, 2019

willjp Oct 26, 2019 •

edited

Loading