/graphpass

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

Implement stdin stdout procedures addressing Issue #47. #62

Closed
wants to merge 16 commits into
base: master
from
+148 −87

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@greebie @ruebot @ianmilligan1
@greebie
Collaborator

greebie commented Sep 19, 2018

GitHub issue(s):

#47

What does this Pull Request do?

Creates a more straightforward stdin stdout procedure call.

e.g. ./graphpass /path/to/input/file /path/to/output/outfile -{flags} will run graphpass on file and store it in outfile.

The stdin and stdout paths can appear anywhere in the expression (you can put {flags} or other options before, between or after the path calls. However, the order of stdin must come before stdout.

The -i or --input will override an stdin call.
The -i or --output will override a stdout call.

Both will have the same behavior as the above mentioned calls.

Graphpass will create an output/ directory in the call ./graphpass /path/to/input/file /path/to/output/outfile -{flags} but only if /path/to/ exists. Otherwise it will throw an error
with a message to create /path/to/.

Any non-flag arguments beyond stdin and stdout will be ignored for now (although graphpass will store them for future use).

How should this be tested?

A description of what steps someone could take to:

  • graphpass should accept the stdin / stdout procedure described above.
  • all tests should pass.
  • it should accept the case where stdout does not exist.
  • no filenames should continue to run the test graphml (currently cpp2.graph in /src/resources).

Additional Notes:

As of writing, I realize that i need to update the documentation in README. I will also check the case where there is no stdout declared.

Interested parties

@ianmilligan1 and @ruebot

Thanks in advance for your help with the Archives Unleashed Project!

greebie added some commits Sep 5, 2018

@greebie
Quit graphpass on graph with > 1000000 edges in the graph.
Loading status checks…
3276477
@greebie
Merge branch 'master' of https://www.github.com/archivesunleashed/gra… 
…phpass
b9804a6
@greebie
First commit to use stdin - stdout. 
- Current unit tests pass.
- Requires full testing.
cd6606d
@greebie
Current first working version (tested with some minor cases).
9c4795c
@greebie
Fix the quickpass test to use ug_OUTPATH instead of ug_OUTPUT.
9b31011
@greebie
Permit non-flag args in any location. 
- you can do this now ./graphpass {stdin} -{Flags} --{OPTION} {OPTION-VALUE} {stdout}
- or ./graphpass -{Flags} --{OPTION} {OPTION-VALUE} {stdin} {stdout]
The only constraint is that stdin comes before stdout.
692f8cf
@greebie
Stdnout now creates a filename as directed by user. 
- If stdout has a '\' at the end, graphpass will use stdin file name.
- Otherwise, stdin will use the filename indicated in stdout.

ex. "/Users/username/filepath/file.graphml /Users/username/OUT/" will put a "file.graphml" in /OUT/.
    "/Users/username/filepath/file.graphml /Users/username/OUT/outfile.graphml" will put "outfile.graphml" in /OUT/ instead.
Loading status checks…
4790ae2
@ruebot

This comment has been minimized.

Show comment
Hide comment
@ruebot

ruebot Sep 19, 2018

Member

Graphpass will create an output/ directory in the call

Why this? Can you explain the rationale here? It's my understanding that stdout should just be stdout. I don't understand the use case of forcing the creation of an output dir.

You also have commits in here from the previous PR.
Member

ruebot commented Sep 19, 2018

Graphpass will create an output/ directory in the call

Why this? Can you explain the rationale here? It's my understanding that stdout should just be stdout. I don't understand the use case of forcing the creation of an output dir.

You also have commits in here from the previous PR.

greebie added some commits Sep 19, 2018

@greebie
Fix for case where no output path is provided. 
- output will default to ./OUT/ where no stdout is provided.
Loading status checks…
74769a8
@greebie
Fix README.md to reflect new command line arguments.
Loading status checks…
cfe99ca
@greebie

This comment has been minimized.

Show comment
Hide comment
@greebie

greebie Sep 19, 2018

Collaborator

Graphpass will create an output/ directory in the call

Why this? Can you explain the rationale here? It's my understanding that stdout should just be stdout. I don't understand the use case of forcing the creation of an output dir.

It will only try to create the output directory if stdout does not exist. Otherwise it will just use stdout.

The way the program is written now, if no stdout exists, and there is no -g flag (to switch to .gexf) the user's original file will be over-written with the graphpassed update. That's why I included code to set the output directory to ./OUT/ if stdout does not exist.

It's possible to fail on no stdout instead, but I think it should be part of a different issue, given that the unit tests work based on the current approach.

You also have commits in here from the previous PR.

I do not understand github properly. Will fix.
Collaborator

greebie commented Sep 19, 2018

Graphpass will create an output/ directory in the call

Why this? Can you explain the rationale here? It's my understanding that stdout should just be stdout. I don't understand the use case of forcing the creation of an output dir.

It will only try to create the output directory if stdout does not exist. Otherwise it will just use stdout.

The way the program is written now, if no stdout exists, and there is no -g flag (to switch to .gexf) the user's original file will be over-written with the graphpassed update. That's why I included code to set the output directory to ./OUT/ if stdout does not exist.

It's possible to fail on no stdout instead, but I think it should be part of a different issue, given that the unit tests work based on the current approach.

You also have commits in here from the previous PR.

I do not understand github properly. Will fix.

greebie added some commits Sep 19, 2018

@greebie
Fix Travis to clean before making. 
(This is to confirm Travis fails are not due to existing compiled tests.)
Loading status checks…
d3bef73
@greebie
Edit make instructions to clean before compile. 
If you do not clean, the qp and ana binaries will not be recompiled and will fail on run.
Loading status checks…
c5527db
@greebie

This comment has been minimized.

Show comment
Hide comment
@greebie

greebie Sep 19, 2018

Collaborator

Getting a seg fault in Travis. Not sure why because it builds fine on my system.
Collaborator

greebie commented Sep 19, 2018

Getting a seg fault in Travis. Not sure why because it builds fine on my system.
@ianmilligan1

This comment has been minimized.

Show comment
Hide comment
@ianmilligan1

ianmilligan1 Sep 19, 2018

Member

I see that it builds fine on my Mac but fails on rho

i.e.

Makefile:27: recipe for target 'install' failed
make: [install] Segmentation fault (core dumped) (ignored)
Member

ianmilligan1 commented Sep 19, 2018

I see that it builds fine on my Mac but fails on rho

i.e.

Makefile:27: recipe for target 'install' failed
make: [install] Segmentation fault (core dumped) (ignored)
@greebie

This comment has been minimized.

Show comment
Hide comment
@greebie

greebie Sep 19, 2018

Collaborator

I see that it builds fine on my Mac but fails on rho

i.e.

Makefile:27: recipe for target 'install' failed
make: [install] Segmentation fault (core dumped) (ignored)

sigh That's what I thought. Going to try and force stdin stdout to see if that fixes things.
Collaborator

greebie commented Sep 19, 2018

I see that it builds fine on my Mac but fails on rho

i.e.

Makefile:27: recipe for target 'install' failed
make: [install] Segmentation fault (core dumped) (ignored)

sigh That's what I thought. Going to try and force stdin stdout to see if that fixes things.
@ruebot

This comment has been minimized.

Show comment
Hide comment
@ruebot

ruebot Sep 19, 2018

Member

It will only try to create the output directory if stdout does not exist. Otherwise it will just use stdout.

That's not how stdout works. We shouldn't be clobbering an existing file if an option isn't given, nor creating a new directory.

I'm going to move on to other stuff for the rest of the day, and will be at York on Thursday and Friday. So, I won't get back to reviewing this until Monday at the earliest.

I do not understand github properly. Will fix.

It's not GitHub issue. It's the process I outlined last week in Slack. Please revisit that.
Member

ruebot commented Sep 19, 2018

It will only try to create the output directory if stdout does not exist. Otherwise it will just use stdout.

That's not how stdout works. We shouldn't be clobbering an existing file if an option isn't given, nor creating a new directory.

I'm going to move on to other stuff for the rest of the day, and will be at York on Thursday and Friday. So, I won't get back to reviewing this until Monday at the earliest.

I do not understand github properly. Will fix.

It's not GitHub issue. It's the process I outlined last week in Slack. Please revisit that.
@greebie

This comment has been minimized.

Show comment
Hide comment
@greebie

greebie Sep 19, 2018

Collaborator

Okay - the issue is that Mac has libgen.h compiled as part of gcc and Ubuntu does not. I will either add instructions to travis re: libgen.h or ignore create a function to get the directory name myself.
Collaborator

greebie commented Sep 19, 2018

Okay - the issue is that Mac has libgen.h compiled as part of gcc and Ubuntu does not. I will either add instructions to travis re: libgen.h or ignore create a function to get the directory name myself.

greebie added some commits Sep 19, 2018

@greebie
Include libgen.h for linux.
Loading status checks…
46b82ab
@greebie
Fix for libgen.h
Loading status checks…
1b3b47a
@greebie
Last commit for this PR. Does not use libgen.h.
Loading status checks…
31d1725
@greebie

This comment has been minimized.

Show comment
Hide comment
@greebie

greebie Sep 20, 2018

Collaborator

That that Travis is hooked up, I am going to close this PR and re-open without the confusing commit problems.
Collaborator

greebie commented Sep 20, 2018

That that Travis is hooked up, I am going to close this PR and re-open without the confusing commit problems.

@greebie greebie closed this Sep 20, 2018

@greebie greebie deleted the issue-47 branch Sep 20, 2018

@greebie greebie referenced this pull request Sep 20, 2018

Open

Change approach to input and output paths. Issue 47 #63

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment