Thanks for this PR, however I have the impression that the use of find is overkill and adds a lot of complexity to map to glob to the regex. My gut feeling the approach shown below should solve the problem

Thank you @pditommaso for reviewing my PR. I tested your proposed solution with the following code.

protected String stageOutCommand( String source, String target, String mode ) {
        final String pattern = removeGlobStar( source )

        if( !pattern )
            return null

        final String escape = Escape.path( pattern )

        return """\
            IFS=\$'\\n'
            for name in \$(eval "ls -1d ${escape.replaceAll(" ","\\ ")}" | sort | uniq); do
                cp -fRLn --parents "\$name" ${Escape.path(target)}
            done
            unset IFS""".stripIndent(true)
}

However, the problem is, that this solution copies more files than necessary, as the removeGlobStar method makes the search term less precise. This will lead to additional IO, as more files are moved or copied to a shared file system. For the move mode, this could also lead to data loss, if a second output is moved to another directory. Moreover, it seems that the script fails, if no file matches the pattern.
For this proposed solution 6 of the 24 cases in the 'should copy the right files' test fail (One due to some special characters, which could be fixed), the others due to copying to many files.
These problems will also apply to the AWSBatchFileCopyStrategy, where more data stored on S3 lead to direct costs.

I think, my proposed glob to regex conversion is a very powerful way to deal with that glob / unix inconsistencies introduced in Nextflow. But you are right, it adds more complexity. But in my opinion, it is the best solution of all three (current state (which does not work for many cases) and adopting the way as you do it for AWS (which copies/moves to much files)).
What do you think?

Why it should copy more files than expected? if I'm understanding correctly your approach converts to glob to a regex and then evaluate it with find, I'm suggesting evaluating the glob with ls command. Avoid the intermediate format would help prevent possible inconsistencies.

Correct me, if I’m wrong, but to evaluate the glob within the ls command, I have to use the removeGlobStar method?
The removeGlobStar method transforms something like 'some/**/path' to 'some*' which will also apply for 'some/a/b', which should not be copied.
That is why I transform the glob into a regex. The regex supports '**' and '*' as they are intended.
Changing the removeGlobStar method in a way, that it replaces '**' with '*' would also lead to cases where more files match the pattern than intended.
'a/**/b*.txt' would then also match 'a/path/blob/data.txt'.
Is that what you were asking for?

Got your point, I was missing the need for **. What about relying on the bash globstar option?

This looks really promising. I just had some tests and it should work. I will adjust my solution on Monday. However, the globstar requires Bash 4.0, does that violate some assumptions of Nextflow?

the best would be trying setting it and fallback to default otherwise, since it would not break it

I updated the code in a way that it uses Bash’s glob instead of converting Java’s glob to a regex. Furthermore, I added more test cases. In contrast to the regex approach it requires Bash 4.0 instead of Shell.
I’m looking forward to your feedback.

Thanks @pditommaso for your feedback.
I understand your concerns regarding unexpected issues, but as I wrote in the PR description the behavior of the staging in Kubernetes is currently in most cases different to the local environment. As it copies either wrong files or the right ones to another path. It only works, if we copy files in the first hierarchy or without glob in path.
For most of the users the behavior I create with that PR is the expected one. That’s why I do not think we should put it into a new, not default option.
To avoid problems with older bash versions, while keeping it working for the current cases, I would suggest making the glob notation optional adding || true:
shopt -s globstar extglob || true
shopt -u globstar extglob || true
Accordingly, it will work for files on the first hierarchy and for files with a concrete path as it does now.

The main problem of this approach is that uses options that are not portable e.g cp -n is not working with the cp command distributed with alpine and busybox. cp --parents does not work in mac. This is was suggesting to add this as an opt-in feature via stageOutMode.

Thanks for the input, I was not aware of both. I removed the -n as it was not required anymore since the last changes where I process all outputs at once.
As the unstage method is only used in cluster environments I do not see issues with MacOS, as this will never be Macs. Am I wrong? If this is the case, it is possible to use rsync instead of cp, if it runs on Mac. Unfortunately I have no Apple available to test something like this.

@pditommaso, I changed the copying logic to avoid the --parents attribute, therefore I simply used the logic which I implemented for the mv command already.
Furthermore, I added a check whether a file was already copied, as it could happen, that we have a glob like **/a/ and a structure like a/a/a/. I forgot this in my last comment.

I needed to make some refactoring to fix another bug (#2141) that overlap with some of this logic. The plan to address the problem in this PR is to change the unstage output code as shown below

If the shopt -s globstar extglob is not supported, the subprocess just ignore it reporting a warning message.

For each of the file resolved, a bash function is invoked depending the stage mode used (copy, move, rsync)

The benefit of this approach is that it follows exactly the same pattern used for aws, azure and google cloud providers. This makes much easier to maintain and in the future, it could be managed in a common abstraction.

Still haven't time to check if it addresses all use cases included in this PR, but I think there's space to improve this implementation. What do you think?

For details check the branch fix-globstar-pattern, in particular the commit 700e4de

Hello @pditommaso, this seems to be exactly what I implemented before. Sure, I can put my logic into methods instead of writing always the copy command, but I do not see more differences.
However, my branch adds also test cases, which perhaps would not all pass here. There are some weird special cases, with special characters in file names. These cases are addressed in my commit.
Maybe you can merge my commit into the fix-globstar-pattern Branch, merging the idea of having everything in methods with my logic around it and the test cases. I think, then we have a very stable solution. Otherwise, I continue working in this branch, and it does not match with yours in the end, as we are really implementing the same. And merging becomes failure-prone.

I've realised that I've forgotten to remove the use normalizeGlobStarPaths. Let's continue this PR using fix-globstar-pattern as base. I'll change it now.

Not surprising there are conflicts. If you have a chance to resolve them it would be great, otherwise, I'll look at my earliest convenience.

I try to merge it next week.

I partially merged this on master 👉 2d4c3df adding a tests for the case reported above

http://github.com/nextflow-io/nextflow/blob/89ce364c93d0a1a2c668004e64ef7c63e496043f/tests/output-globs.nf#L1-L1

However, I've stripped the support for extended globs, ie. **, since I could not tests carefully. Likely makes more sense to close this PR and eventually reopen for the part regarding the extended globs.

Sorry, @pditommaso, I did not yet find the time to merge it, I planned to do it tomorrow.
For me, especially the ** is an important feature, why your solution does not fix all my problems.
I think it is a big issue if Nextflow does not find the same files in a distributed environment than locally. That makes it very hard to debug a Nextflow Workflow.
I see you copied some of my ideas, but you didn’t merge them, so you missed the test cases. I think especially my newly added tests are important to improve Nextflow’s stability. As the current tests only check the generated scripts, but never their results.
I propose that I open a new PR, adding my test cases. The test cases might fail, due to the missing ** support. Furthermore, I spend a lot of time, doing my contribution, checking for special char combinations - I assume also some failures here, but let’s see.
After I reincluded the tests, we can look together for a solution to solve the ** problem, as you didn’t like my proposed one.
How do you think about that? I hope to start tomorrow.

Sure, I understand that. Just wanted to notify you on the current status. I agree, that at this stage it would be better to open a new PR. Thanks a lot.

I opened two new PRs, #2181 adding the test cases from this PR and #2182 where I implemented the fix for the extended glob.
I think this PR can be closed.

Thanks