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

make new release? #15

Closed
wants to merge 13 commits into from
Closed

make new release? #15

wants to merge 13 commits into from

Conversation

vn971
Copy link

@vn971 vn971 commented Oct 31, 2018

Sorry for raising it like this... But are you aware of the existence of forks? If you could merge the functionality and make a new release, it'd be great!
The version in this pull request builds on stable Rust.

// I've temporarily forked the lib on crates.io using the name "libalpm-fork" (https://github.com/vn971/alpm) , but I'd happily delete it if upstream release will be made.

@richard-uk1
Copy link
Owner

I'm more than happy to accept PRs, or to make the repository shared ownership. I'm pretty quick at responding to them as well (usually).

@richard-uk1
Copy link
Owner

I'm also open to making other people (you?) have shared ownership of the repo/crate so you don't need to wait for me.

@@ -0,0 +1,7 @@
<component name="ProjectDictionaryState">

Choose a reason for hiding this comment

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

What is this file for?

Copy link
Author

Choose a reason for hiding this comment

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

It's an "intellij idea" file. No use for compiling the project, but can supposedly help other people who use the same IDE.

Copy link
Author

Choose a reason for hiding this comment

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

FTR, not my file, but rather one added by gustorn (the first fork of libalpm).

Choose a reason for hiding this comment

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

How can it help? It's some kind of dictionary that is not necessary for idea. Also it seems it's gustorn's personal file since it has same name. I'd rather remove it from PR before merge.

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I think it can be wiped out, especially since the dictionary contains 1 item only.
Done.

@@ -12,7 +12,7 @@ An aspirationally safe wrapper around libalpm (the Arch Linux Package Manager).
"""

[dependencies]
alpm-sys = "0.1"
alpm-sys = { git = "https://github.com/jameslzhu/alpm", branch = "master" }

Choose a reason for hiding this comment

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

This should probably be reverted in the crates.io release.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, among other stuff. Now that I have approve from derekreery and everyone, I'll make actual changes. (Besides changes done in "-fork" to publish it, which will not be needed once we make upstream release anyway.)

@jameslzhu
Copy link

jameslzhu commented Nov 2, 2018

Hello. I am one of the contributors in the PR commits. Actually, it seems @vn971 initiated the pull request from my fork.

I'm uncomfortable that @vn971 initiated this PR, from my fork, without my consent, but more so that he published my (almost entirely unmodified) changes to crates.io without my approval.

That said, I'm willing to have my code merged upstream, so I'm going to go ahead and approve my contribution to this PR. I'm afraid I'm too busy right now to accept any kind of maintainership responsibilities (I don't even know if my code still compiles on stable).

That said, there are two separate things that should be done:

  • Yanking the libalpm-fork from crates.io.
  • Changing the fork compare branch from jameslzhu:master to vn971:master

There are going to be a few changes needed (the dependency in libalpm/Crates.toml links to my git repo, instead of the versioned crate), and that will require changing the PR from my fork to @vn971's fork so he can make the changes.

@vn971 in the future, consider using a git dependency instead of publishing a fork on crates.io:
https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories

[dependencies]
rand = { git = "https://github.com/rust-lang-nursery/rand" }

I know piston and serde used these for quite some time.

@vn971
Copy link
Author

vn971 commented Nov 2, 2018

Hi @jameslzhu. I do not understand why you are uncomfortable with me publishing code to crates.io. Your contributions seem to have the MIT license, which I fully respect. I do not intent to presume authorship over the code contributions you made either. Specifying a "git" dependency did not work for my project, which uses libalpm. crates.io refuses to publish any such project by saying that git dependencies are not allowed (or something like that). I do not have the exact wording as I've already moved to using the "-fork" version, though I can revert to the "git" version if it's really that important. Maybe it's OK to use "git" dependencies under certain circumstances that my project did not meet.

I will definitely change the PR from your master branch to mine though.

@jameslzhu
Copy link

Hi @vn971,
I've thought over what I just said, and I'll retract them. (It's a little late in my corner of the world.) I can't reasonably expect to have control over my code if it's published openly, under the MIT license. I suppose I just wasn't expecting to have my code published on crates.io so suddenly. You have my full permission and consent.
I trust you're correct on the git dependency issue. Otherwise, good luck in your endeavors.

@vn971
Copy link
Author

vn971 commented Nov 2, 2018

Moved the PR here: #16 We need this anyway as a way to fix e.g. travis issues, among others.

@vn971 vn971 closed this Nov 2, 2018
@vn971
Copy link
Author

vn971 commented Nov 2, 2018

@jameslzhu I understand why this might feel offensive. Hopefully everyone would be happy with the main crates.io package being updated (and compile-able on stable Rust), you added to authors in Cargo.toml, "-fork" being deleted as planned etc.

@richard-uk1
Copy link
Owner

I'd like to bring https://github.com/derekdreery/alpm-experimental to your attention - I was experimenting with writing my own alpm lib in rust, and reading through the C code I was kinda not impressed. I think there's definitely potential in rewriting alpm (I'm probably 50% there), and maybe even offering the libalpm C api from our rust version, so it could be a drop-in replacement.

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.

5 participants