Skip to content

Conversation

jfennick
Copy link
Contributor

This PR (hopefully) fixes #1029.

@mr-c
Copy link
Member

mr-c commented Jul 13, 2022

Thanks for this! The lint error is being fixed over in #1691

@jfennick
Copy link
Contributor Author

It looks like my change is breaking some other examples, but I'm a bit lost. Is anyone willing to take a look and help push this PR across the finish line?

@mr-c
Copy link
Member

mr-c commented Jul 14, 2022

Here is one of the conformance tests, that now fail

cwltool v1.0/cat-from-dir.cwl v1.0/cat-from-dir-with-literal-file.yaml
Pipe to stdin from literal File via a Directory literal
Returned non-zero
ERROR'cat' not found: [Errno 2] No such file or directory: "I'm a File literal; howdy!"
WARNING[job cat-from-dir.cwl] completed permanentFail
WARNINGFinal process status is permanentFail

@jfennick
Copy link
Contributor Author

Yes, I read through the logs. I was asking if any of the maintainers are willing to help fix the errors.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Debugged the test @mr-c linked, and suggested a possible fix for @jfennick.

@jfennick maybe we should include the test from #1029 as a unit test here?

Hope that helps!
-Bruno

@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #1690 (f039acd) into main (b126a46) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1690   +/-   ##
=======================================
  Coverage   66.85%   66.85%           
=======================================
  Files          93       93           
  Lines       16670    16692   +22     
  Branches     4422     4428    +6     
=======================================
+ Hits        11145    11160   +15     
- Misses       4380     4386    +6     
- Partials     1145     1146    +1     
Impacted Files Coverage Δ
cwltool/job.py 79.48% <100.00%> (-0.40%) ⬇️
cwltool/pathmapper.py 93.47% <100.00%> (+0.79%) ⬆️
cwltool/process.py 88.95% <100.00%> (+0.01%) ⬆️
cwltool/cwltool/job.py 61.46% <0.00%> (-1.19%) ⬇️
cwltool/cwltool/process.py 71.01% <0.00%> (+0.04%) ⬆️
cwltool/command_line_tool.py 78.85% <0.00%> (+0.13%) ⬆️
cwltool/cwltool/pathmapper.py 86.95% <0.00%> (+1.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b126a46...f039acd. Read the comment docs.

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Looks great! It will need a confirmatory test.

@mr-c
Copy link
Member

mr-c commented Jul 14, 2022

Try make install-dep format

@@ -191,6 +192,16 @@ def files(self) -> List[str]:
def items(self) -> List[Tuple[str, MapperEnt]]:
return list(self._pathmap.items())

def items_exclude_children(self) -> List[Tuple[str, MapperEnt]]:
Copy link
Member

Choose a reason for hiding this comment

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

FYI @tetron @DailyDreaming @kellrott about this proposed addition to PathMapper

@jfennick
Copy link
Contributor Author

Try make install-dep format

Sorry for blasting your CI for trivial formatting, but I was running into nasty installation errors when trying to run it locally using the instructions in the readme. If I can reproduce, I'll try to open another issue.

@mr-c
Copy link
Member

mr-c commented Jul 15, 2022

Try make install-dep format

Sorry for blasting your CI for trivial formatting, but I was running into nasty installation errors when trying to run it locally using the instructions in the readme. If I can reproduce, I'll try to open another issue.

S'alright. New commits will cancel old CI jobs automatically.

Did you have a dedicated Python virtualenv active before running those commands?

@jfennick
Copy link
Contributor Author

Try make install-dep format

Sorry for blasting your CI for trivial formatting, but I was running into nasty installation errors when trying to run it locally using the instructions in the readme. If I can reproduce, I'll try to open another issue.

S'alright. New commits will cancel old CI jobs automatically.

Did you have a dedicated Python virtualenv active before running those commands?

I created a new conda environment. See #1694

@mr-c mr-c merged commit d4d4515 into common-workflow-language:main Jul 15, 2022
@jfennick jfennick deleted the symlink_exclude_children branch March 9, 2023 04:45
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.

Input directory corruption and infinite symlink using successive InitialWorkDirRequirement
3 participants