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

Return values for other modes. Functions for use in external tools. #8

Closed
wants to merge 3 commits into from

Conversation

brenthuisman
Copy link
Contributor

  • Other modes (create, repair) also return a par2cmdline-compatible return value.
  • Expose a function par() that ingests a []string which can be used exactly as the cmdline interface.
  • Expose a C function if compiled with -buildmode=c-shared
  • Fixes libgopar #6.

Copy link
Owner

@akalin akalin left a comment

Choose a reason for hiding this comment

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

Ah, I didn't realize you wanted to embed this via a C api! That's fine, just some initial comments about where the C/unsafe code should go.

cmd/par/main.go Outdated Show resolved Hide resolved
cmd/par/main.go Outdated Show resolved Hide resolved
cmd/par/main.go Outdated Show resolved Hide resolved
@brenthuisman
Copy link
Contributor Author

I think it's up to shape now!

@akalin
Copy link
Owner

akalin commented Feb 6, 2021

Hi Brent,

Okay, I finally had some time to get back into gopar stuff and to take a look at this, and I have some good news and bad news.

Bad news first: the more I think about it, the more I'm convinced that exposing a single function gopar() like this isn't the right way to expose gopar as a C library. I'm not ruling it out yet, but I'd like to see where exactly it would be used first, and then I can probably come up with some suggestions.

Some reasons why I think this isn't the best fit:

  • it sounds like you only need par2 functionality, and yet the command-line app (and the shared library) handles par1 too.
  • if the only interface is a single function that takes in strings, there's some extra translation that has to go on on both sides of the API.
  • If other Go projects want to use gopar, I think they can just use the par1 or par2 packages directly. The command-line app is meant to be a thin shell over those packages -- I think there are some default values that live in the main package, but I think I want to move those to par1 or par2.
  • If other C/Python projects want to use gopar, it's likely they'll want to do something different from what you want, and so I prefer to handle those as they come.

So if we come up with a custom API, that'll solve all the above, I think.

I took a look at https://github.com/brenthuisman/par2deep/tree/master/par2deep and didn't find anything referring to gopar -- I presume you're working on it in a local branch? If you could upload it / give me access to it, I can take a look at how you're using it, and then we can hammer out an interface.

The good news is that I'm "paging in" all of the stuff I forgot about gopar, and I now have some time to work on it (although mostly on the weekend, with some nights).

So my thinking about how this can go is:

  1. I'm guessing you're calling gopar either by shelling out to the command line, or calling the shared library from this branch. That can work for now as we figure out the rest.
  2. Once I look at your code, I can see what API methods you need, and can update this branch or whatever, and then you can update your code to match.
  3. Once we're both satisfied, I can merge this into master.

@brenthuisman
Copy link
Contributor Author

brenthuisman commented Feb 8, 2021

* it sounds like you only need par2 functionality, and yet the command-line app (and the shared library) handles par1 too.

True, but that does not make it less of a solution. par2deep happens to not expose par1 functionality, but other tools could.

* if the only interface is a single function that takes in strings, there's some extra translation that has to go on on both sides of the API.

* If other Go projects want to use gopar, I think they can just use the par1 or par2 packages directly. The command-line app is meant to be a thin shell over those packages -- I think there are some default values that live in the main package, but I think I want to move those to par1 or par2.

This is a view that matches those of the (various forks of) par2cmdline. I think those API are too low level and hence hampers adoption in other tools. I think it is no surprise other tools invariable include a binary copy of a compiled version of par2cmdline and then interface with that, rather than use 'internal' libpar2 APIs.

My reasons for having an API identical to the cmdline interface (i.e. passing in the same strings) has three benefits:

  1. A consistent way to interact with the tool/library.
  2. Makes it easy to swap out the various par forks in case the user desires so, something still supported by par2deep.
  3. It's easy to implement ;)

A better API would be Create(), Verify(), Repair() functions that take say a dict of options + filehandle/datahandle, as similar as possible to the ones you pass at the commandline. I'd be interested in this. Since Go is new to me and because time is limited, I however haven't taken a stab at it yet.

* If other C/Python projects want to use gopar, it's likely they'll want to do something different from what you want, and so I prefer to handle those as they come.

Tools that currently do (they're usually related to downloading software with a certain provenance, so it may be best not to link) include binaries and shell out. par2deep used to (and still can), in addition to using a supplied library.

So if we come up with a custom API, that'll solve all the above, I think.

I took a look at https://github.com/brenthuisman/par2deep/tree/master/par2deep and didn't find anything referring to gopar -- I presume you're working on it in a local branch? If you could upload it / give me access to it, I can take a look at how you're using it, and then we can hammer out an interface.

You're right that the gopar fork for now only lives on my drive, but the difference is really only in the Create() flags that gopar (doesn't) support. This function is calling par2cmdline, or the lib. You can see why a string interface to the lib makes the implementation flexible ;)

https://github.com/brenthuisman/par2deep/blob/master/par2deep/par2deep.py#L98

The good news is that I'm "paging in" all of the stuff I forgot about gopar, and I now have some time to work on it (although mostly on the weekend, with some nights).

So my thinking about how this can go is:

1. I'm guessing you're calling gopar either by shelling out to the command line, or calling the shared library from this branch. That can work for now as we figure out the rest.

My plan was to take gopar as an excuse to learn Go a bit more in depth and port par2deep. But two months have passed since I developed this plan, so maybe I'll update current par2deep with a soft fork of gopar, and continue when time permits with the rest.

2. Once I look at your code, I can see what API methods you need, and can update this branch or whatever, and then you can update your code to match.

Very little is the good news! Here an example of runpar() usage:

https://github.com/brenthuisman/par2deep/blob/master/par2deep/par2deep.py#L324

3. Once we're both satisfied, I can merge this into master.

So, I guess I'll close it. The Create(), Verify(), Repair() that I think makes most sense for a proper library API would match the ones in the cmdline interface, so the flagsetting is the thing that would need to be changed a bit; they each need to set their own state based on the input settings dict/struct. Then making /cmd/par be a consumer of the library makes sense, right? Maybe we could work out that API a bit more in the Issue? Your last message is more or less what I have in mind, apart from maybe wanting to pass in a filehandle rather than a filename and me not knowing this delegate pattern ;)

@akalin
Copy link
Owner

akalin commented Feb 8, 2021

Ok, cool, so I think we're in agreement that the ideal solution would be a Go API that more closely mirrors the command-line interface (e.g. Create, Verify, Repair). I think we talked about it before, but due to time constraints we tried the 'single function' API first.

So now that I have some time, I'l try working on that, hopefully next weekend. I won't do the cgo part yet -- I figure we can tackle that as a follow-up. 👍

@brenthuisman
Copy link
Contributor Author

That's great! Don't hesitate to ping me to test / take care of the cgo part!

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.

libgopar
2 participants