Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for "output discrepancy if file already exists" (Issue #12) #14

Merged
merged 3 commits into from
Jun 24, 2016

Conversation

romangrothausmann
Copy link
Contributor

Possible fix for Issue #12, please check.
Yields the correct dependencies for the example Makefile in these cases:

touch x.a
touch x.a x.b
touch x.a x.b x.z
touch x.b
touch x.z
touch x.a x.b x.z; rm x.a
touch x.a x.b x.z; rm x.z

but not in this:

touch x.a x.b x.z; rm x.b

@lindenb
Copy link
Owner

lindenb commented Jun 20, 2016

Thanks for your PR.
I tried your version with tabix : https://sourceforge.net/projects/samtools/files/tabix/

this is the output I get : https://github.com/lindenb/makefile2graph/blob/master/screenshot.png

and below is your output:

jeter

as you can see, your version contains many circular dependencies.

@romangrothausmann
Copy link
Contributor Author

Hm, I'll have a look.
Do You think the general idea for the indentation parsing is implement sensibly?
At least, refering to #12, I figured, that it was not the Pruning file x.a but the Considering target file 'x.a'. issued before Considering target file 'x.b'. that caused the wrong dependency.

@romangrothausmann
Copy link
Contributor Author

Using the same comparison as in L283 makes the output for both (#12 and tabix) look correct.

test0

test1

Even touch x.a x.b x.z; rm x.b is correct, I just was irritated by the disconnected x.a but as it exists and needs no updating, the output is right.

test1

I'm just not sure whether an indentation condition should be added to other else if(startsWith(. Perhaps @deryni can comment on this.

@lindenb lindenb merged commit 2a9c327 into lindenb:master Jun 24, 2016
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.

2 participants