marxin / cvise

Super-parallel Python port of the C-Reduce
Other
233 stars 25 forks source link

Allow folders/directories in test cases #110

Closed dmcdougall closed 6 months ago

dmcdougall commented 1 year ago

This issue isn't really an issue in the true sense of the word. I'm just opening an issue because I wasn't sure where else to get some insight.

I noticed that passing test case locations that aren't in the current directory, and therefore contain a directory, are not supported. Is there a technical reason for this limitation? I use multidelta quite a bit. My workflows usually involve minimising complex applications which often come with source trees that aren't necessarily "flat", and multidelta is happy to accept testcases with folders in the path.

I'm happy to poke around in the cvise source code and contribute a patch that would allow users to pass test cases that contain directories, but I just wanted to make sure that there wasn't some historical context behind the limitation first. Perhaps someone has already tried this and it was too awkward to support? Any insight would be valuable.

marxin commented 1 year ago

This issue isn't really an issue in the true sense of the word. I'm just opening an issue because I wasn't sure where else to get some insight.

First, thanks for the issue, appreciate that!

I noticed that passing test case locations that aren't in the current directory, and therefore contain a directory, are not supported. Is there a technical reason for this limitation?

Yes, the main limitation is that there are concurrent reduction candidates of the reduced files and each of them is copied into a temporary folder. And that's why C-Vise does not modify a source file(s) in place. However, I've faced similar challenge you have multiple times in the future and I always made a workaround:

I use multidelta quite a bit. My workflows usually involve minimising complex applications which often come with source trees that aren't necessarily "flat", and multidelta is happy to accept testcases with folders in the path.

Fully makes sense.

I'm happy to poke around in the cvise source code and contribute a patch that would allow users to pass test cases that contain directories, but I just wanted to make sure that there wasn't some historical context behind the limitation first. Perhaps someone has already tried this and it was too awkward to support? Any insight would be valuable.

No, I haven't made any attempt and I would welcome a patch regarding this problem. I can imagine a new option that would make a copy of a given folder to the temporary file. Or do you have any better solution? Please, keep in mind the reduction happens in parallel and so an application folder can't interact in between parallel processes.

dmcdougall commented 1 year ago

I've faced similar challenge you have multiple times in the future and I always made a workaround

Great minds think alike. I've also implemented similar workarounds for my use cases.

I can imagine a new option that would make a copy of a given folder to the temporary file. Or do you have any better solution?

That's the solution I had in mind. Take the user-provided test case, which contains a folder, and strip the filename. There are tools like basename that do this in UNIX so I'm sure there are spiritual equivalents provided by the path module in python. Then have cvise do the equivalent of mkdir -p <part of path containing only the directories> inside the temporary test directory and copy the test case there.

I can probably quite easily make a patch for this if you're interested.

marxin commented 1 year ago

All right, so I made a prototype in the following branch: https://github.com/marxin/cvise/tree/add--copy-extra-folder

It adds a new argument --copy-extra-folder that copies content (recursively) of a directory to a temporary place where inter. test is run. Note the reduced test-cases may be contained in the folder and it can be also . folder if you want. Please test it and give me a feedback if it's feasible or not.

dmcdougall commented 1 year ago

I didn't expect the entire contents of the tree to be copied over, just the test case and supporting folder structure. Maybe this doesn't matter, though.

marxin commented 1 year ago

Oh, you expected only the supporting folder structure. So can you please reduce my patch candidate, probably rename the option, test it, and make a pull request that would fill your needs? Thanks.

dmcdougall commented 1 year ago

Sure, I'd be happy to.

dmcdougall commented 6 months ago

My attempt is in #132.

Constructive criticism is very welcome.

dmcdougall commented 6 months ago

Fixed in #133