Windows Compatibility: ensuring Cwltool works on windows OS #419

kapilkd13 · 2017-06-09T13:50:09Z

This is the main branch for windows compatible cwltool.

…heme

… to true

This reverts commit b5882c5.

kapilkd13 · 2017-06-12T23:01:03Z

Hi, I am working on windows compatibility for cwltool. I am getting file not found error in TestFactory's test_partial_scatter and test_partial_output see here https://ci.appveyor.com/project/mr-c/cwltool/build/.7-master/job/usc0vga9xhaj7e39#L945 There are few requirements in cwl files of these tests like StepInputExpressionRequirement and ScatterFeatureRequirement see here and here.
I want to know if either of these testcases depends on symlinks or any unix dependent feature(except unix tools like echo,touch etc). Once I know this I can completely focus on debugging it. Thanks

manu-chroma · 2017-06-14T06:34:10Z

appveyor.yml

+
+
+test_script:
+  - "%CMD_IN_ENV% python setup.py test"


you can add a command before calling setup.py test to install requirements from pip.

Rather than modifying setup.py, replace schema_salad in requirements.txt to
https://github.com/kapilkd13/schema_salad/archive/windows#egg=schema_salad-2.4.20170612171942

this will be enough. you can remove dependency_link from setup.py if you use this approach.

Currently its just for testing, until schema-salad PR get merged. Its working fine right now but if any problem occur I will go this way. I liked the idea of tweaking requirements.txt rather than setup file.

yeah, I feel the same. to just specify the version in setup.py 😄

tetron · 2017-06-14T16:31:11Z

StepInputExpressionRequirement and ScatterFeatureRequirement do not depend on symlinks or any unix features. They are internal operations on the workflow graph.

InplaceUpdate is an extension and doesn't need to be supported on Windows.

…ncoding on windows

kapilkd13 · 2017-06-15T17:48:41Z

Thanks @tetron It helped 👍 Now we are passing all tests on appveyor. 😺

kapilkd13 · 2017-06-15T18:04:25Z

Hi @manu-chroma As you are working on python3 compatibiltiy, I think you can help me with this https://travis-ci.org/common-workflow-language/cwltool/jobs/243363735 travis failing test. It is saying too few args for lchmod but lchmod takes 2 args and are provided. Maybe I am missing something. Thanks

manu-chroma · 2017-07-17T08:44:08Z

@kapilkd13 It's the python 3 builds which are failing. The Queue module has been renamed to queue in Python 3. this should help:

try:
    import queue #python3
except ImportError:
    import Queue as queue

…te path on windows

manu-chroma · 2017-07-17T09:05:29Z

@kapilkd13, since you're using Queues with threads, I think it's better to use #419 (comment) rather than multiprocessing Queues.

You might find this useful: https://stackoverflow.com/a/925241/1180321

kapilkd13 · 2017-07-17T09:09:16Z

Okay. I would do that. Thanks for help help 😄

kapilkd13 · 2017-07-17T10:36:05Z

Thanks @manu-chroma 👍

manu-chroma · 2017-07-17T11:08:43Z

@mr-c Would it make sense to do a release now that Python 3 is supported before merging this one?
If not on PyPI, a GitHub tag and release would make sense IMO. Thoughts?

mr-c · 2017-07-17T11:15:29Z

cwltool/job.py

            runtime.append(u"--user=%s" % (euid))

        if rm_container:
            runtime.append(u"--rm")

-        runtime.append(u"--env=TMPDIR=/tmp")
+        runtime.append(u"--env=TMPDIR=%s" % self.builder.tmpdir)


no, the tmpdir inside the docker container shouldn't change

Okay. I thought we hardcoded tmp by mistake. I will undo it.

mr-c · 2017-07-17T11:19:10Z

cwltool/job.py

@@ -436,7 +450,7 @@ def _job_popen(

        sp = subprocess.Popen(commands,
                              shell=False,
-                              close_fds=True,
+                              close_fds=False,


That's a big change :-) How about close_fds=not onWindows(),?

mr-c · 2017-07-17T11:34:51Z

Hey @kapilkd13

From the list below, what versions of Windows + docker have been tested?

Operating System :: Microsoft :: Windows :: Windows 10
Operating System :: Microsoft :: Windows :: Windows 3.1 or Earlier
Operating System :: Microsoft :: Windows :: Windows 7
Operating System :: Microsoft :: Windows :: Windows 8
Operating System :: Microsoft :: Windows :: Windows 8.1
Operating System :: Microsoft :: Windows :: Windows 95/98/2000
Operating System :: Microsoft :: Windows :: Windows CE
Operating System :: Microsoft :: Windows :: Windows NT/2000
Operating System :: Microsoft :: Windows :: Windows Server 2003
Operating System :: Microsoft :: Windows :: Windows Server 2008
Operating System :: Microsoft :: Windows :: Windows Vista
Operating System :: Microsoft :: Windows :: Windows XP

kapilkd13 · 2017-07-17T11:41:16Z

@mr-c Windows 10+native docker for windows and Windows 8.1+ docker toolbox have been tested. I won't be fully sure, but as it works with Docker Toolbox, I would say it should also support Win 8 and win 7.

mr-c · 2017-07-17T11:41:49Z

@manu-chroma I'm making a separate release first, yep :-) See #472

…s or requirement

kapilkd13 · 2017-07-17T15:31:26Z

@mr-c I think we are ready to go. That container issue is resolved. I removed additional code.

mr-c

Otherwise this looks great!

mr-c · 2017-07-17T17:41:46Z

tests/util.py

@@ -15,3 +17,8 @@ def get_data(filename):
    if not filepath or not os.path.isfile(filepath):
        filepath = os.path.join(os.path.dirname(__file__), os.pardir, filename)
    return filepath
+
+# Check if we are on windows OS
+def onWindows():


why not use the version in utils.py?

You mean windows platform version for win 8 and 10. something like sys.platform==win32 etc?

https://github.com/common-workflow-language/cwltool/pull/419/files#diff-172ce10c584f6a72521c6305db028c9bR107

Oh right 😄 I thought we put all test utilities in util file inside test folder. Earlier i was using this only. i will switch.

kapilkd13 added 18 commits June 5, 2017 09:11

added appveyor.yml script

bee9324

normalizing input path depending on OS

1285c1b

windows compatibility issue: windows Drive names are considered as sc…

85fa796

…heme

all changes towards windows compatibility

9db2091

setting close_fds as true

ffe3aad

disabling auto cleanup onn namedtempfile closeup

9a5221c

separate function for copying file and directory and seting close_fds…

fb42e6a

… to true

using shutil.rmtree to remove dirs

2f9ac55

checking dirs and removing them before copytree

8d3ed5c

using os.mkdir instead of tempfile.mkdtemp

b5882c5

Revert "using os.mkdir instead of tempfile.mkdtemp"

eeee83b

This reverts commit b5882c5.

modified stagefile func

f957ad8

odified copytree func and allowing staged files to copy

2118bd4

enabled duplicate test

0b601fd

separating windows and linux specific code

7902707

windows issue: environment var can only contain strings

5ddf49f

adding urljoin function else will get NotImplemented error

367756a

subprocess executionn on windows need TEMP var always set

236a217

kapilkd13 added 2 commits June 13, 2017 16:38

muting InplaceUpdate testcases on windows platform with pytest

b546f01

pulling schema-salad from local github repo

5bca2e0

manu-chroma reviewed Jun 14, 2017

View reviewed changes

kapilkd13 added 3 commits June 14, 2017 23:46

checking for path existence instead of / path begining

c311908

modified wf/echo.cwl, removing newline character which has differnt e…

bdebbc6

…ncoding on windows

new sha for test_partial_output

4dc76bd

added new sha for test_scatter

c63c4dd

kapilkd13 force-pushed the testing-windows branch from 3f2eaa1 to c63c4dd Compare June 15, 2017 17:54

Merge branch 'master' into testing-windows

681da6b

kapilkd13 added 2 commits July 17, 2017 14:28

taking care of case when dockeroutputdirectory provided is not absolu…

550c406

…te path on windows

Py 3 doesn't have Queue module, use multiprocessing.queue instead

5bdc1ef

kapilkd13 and others added 3 commits July 17, 2017 14:59

removed unused import urlparse from test_fetch

4954f2c

tests/wf/echo.cwl: encode string to bytes before writing to binary file

e9f01e2

cwltool/sandboxjs.py: fix mypy errors due to import statement

cfa1990

kapilkd13 changed the title ~~[WIP]Testing windows: not for merge~~ Windows Compatibility: ensuring Cwltool works on windows OS Jul 17, 2017

mr-c reviewed Jul 17, 2017

View reviewed changes

switch back to hardcoded /tmp as TMPDIR inside Docker

1dd8476

set close_fds to false on Windows else True

9267120

Merge branch 'master' into testing-windows

c85ba19

mr-c and others added 4 commits July 17, 2017 15:10

Merge branch 'master' into testing-windows

b203750

checking for default container when container is not provided in hint…

a0c2dae

…s or requirement

removing code that treats default container as lowest priority Hints

85f3299

checking availability of default container before accessing it

b63d472

Merge branch 'master' into testing-windows

8d3f692

mr-c requested changes Jul 17, 2017

View reviewed changes

removing onwindows redundant function from test folder's util.py

06dd0b2

mr-c approved these changes Jul 17, 2017

View reviewed changes

mr-c merged commit 000b07c into common-workflow-language:master Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Compatibility: ensuring Cwltool works on windows OS #419

Windows Compatibility: ensuring Cwltool works on windows OS #419

kapilkd13 commented Jun 9, 2017 •

edited

Loading

kapilkd13 commented Jun 12, 2017

manu-chroma Jun 14, 2017

manu-chroma Jun 14, 2017

kapilkd13 Jun 14, 2017

manu-chroma Jun 14, 2017 •

edited

Loading

tetron commented Jun 14, 2017

kapilkd13 commented Jun 15, 2017

kapilkd13 commented Jun 15, 2017

manu-chroma commented Jul 17, 2017

manu-chroma commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

manu-chroma commented Jul 17, 2017

mr-c Jul 17, 2017

kapilkd13 Jul 17, 2017

mr-c Jul 17, 2017 •

edited

Loading

mr-c commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

mr-c commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

mr-c left a comment

mr-c Jul 17, 2017

kapilkd13 Jul 17, 2017

mr-c Jul 17, 2017

kapilkd13 Jul 17, 2017

Windows Compatibility: ensuring Cwltool works on windows OS #419

Windows Compatibility: ensuring Cwltool works on windows OS #419

Conversation

kapilkd13 commented Jun 9, 2017 • edited Loading

kapilkd13 commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manu-chroma Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

tetron commented Jun 14, 2017

kapilkd13 commented Jun 15, 2017

kapilkd13 commented Jun 15, 2017

manu-chroma commented Jul 17, 2017

manu-chroma commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

manu-chroma commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-c Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

mr-c commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

mr-c commented Jul 17, 2017

kapilkd13 commented Jul 17, 2017

mr-c left a 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

kapilkd13 commented Jun 9, 2017 •

edited

Loading

manu-chroma Jun 14, 2017 •

edited

Loading

mr-c Jul 17, 2017 •

edited

Loading