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

New package: EzXMLPatched v1.0.0 #10325

Closed

Conversation

JuliaRegistrator
Copy link
Contributor

@JuliaRegistrator JuliaRegistrator commented Feb 29, 2020

JuliaRegistrator referenced this pull request in aminya/EzXMLPatched.jl Feb 29, 2020
@github-actions
Copy link
Contributor

Your new package pull request met all of the guidelines for auto-merging and is scheduled to be merged when the mandatory waiting period has elapsed.

Since you are registering a new package, please make sure that you have read the package naming guidelines: https://julialang.github.io/Pkg.jl/dev/creating-packages/#Package-naming-guidelines-1


If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment.

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 29, 2020

@KristofferC does this address all of your concerns in #10277 (comment)?

@JuliaRegistrator JuliaRegistrator force-pushed the registrator/ezxmlpatched/ebbc4602/v1.0.0 branch from 97a8164 to 7b8f2e6 Compare February 29, 2020 23:38
JuliaRegistrator referenced this pull request in aminya/EzXMLPatched.jl Feb 29, 2020
@DilumAluthge DilumAluthge requested a review from KristofferC March 1, 2020 01:34
@felipenoris
Copy link
Contributor

felipenoris commented Mar 1, 2020

I've been following the work of @bicycle1885 for around 2 years since I needed a package to handle XML files. I would argue that you have the right to create a fork of EzXML.jl since it is licensed under MIT License. But when it comes to register in the General registry, I would say I see a red flag here:

In my opinion, this is the case for a fork: just fork the package, patch it as you please, and use it.
But it is not the case for registering a new package in the registry.

@DilumAluthge
Copy link
Member

DilumAluthge commented Mar 1, 2020

In my opinion, this is the case for a fork: just fork the package, patch it as you please, and use it.

Just to expand on this answer: You can make your own forks of packages (assuming that they have open-source licenses) and use your forks without needing to register your forks or maintain a registry. Simply use the Manifest.toml file to specify the forks and branches that you want to use, and then check your Manifest.toml file into source control. (You will need to make sure that you do not have any entries in your .gitignore file that match Manifest.toml.)

For example, you can take a look at what Keno has done with the manifest for his unregistered package ToyFHE.jl:

As you can see in the manifest file, Keno has forked several of the repos, and ToyFHE.jl uses Keno's forks instead of the upstream repos. As one example, instead of using the upstream BitIntegers.jl repo (https://github.com/rfourquet/BitIntegers.jl), the manifest uses the kf/div branch inside Keno's fork (https://github.com/Keno/BitIntegers.jl).

Of course, you should not create or edit Manifest.toml files by hand. So doing something like ]add https://github.com/Keno/BitIntegers.jl#kf/div in the Pkg REPL mode is how you tell Pkg to use your fork and your branch.

@aminya
Copy link
Contributor

aminya commented Mar 1, 2020

you claim in #10277 that bicycle1885 may not want to maintain the package anymore. But he has been looking at your issue (bicycle1885/EzXML.jl#125) and even sent the correction upstream (JuliaLang/julia#34941).

My PR has been open since last year.

you created a fork with a broken CI. This makes me feel that maybe bicycle1885 is doing a better job in maintaining a community package.

I am using Github Actions.

you're really registering this for the wrong reason: you just disagree with bicycle1885 that it is Ok to show Ptr addresses in a show method.

Ptr.address causes errors + slows down showing. https://github.com/aminya/AcuteML.jl/blob/ec35bba2ae5ad86f9b0c1b1a8ee06d6563e9280c/benchmark/bench.jl#L88

In my opinion, this is the case for a fork: just fork the package, patch it as you please, and use it.
But it is not the case for registering a new package in the registry.

This is meant for the internal use of AcuteML.jl, you are not forced to use EzXMLPatched.jl. If Pkg could allow me to include a fork inside the Project.toml, I didn't need registering anymore.

@felipenoris
Copy link
Contributor

felipenoris commented Mar 1, 2020

@aminya , I share your frustration when you find a community package that does not satisfy your needs. Your claim is based on a 4-month old PR that triggers an upstream error on an unreleased Julia version. You just reforce my argument that this is the case for a fork and not for a new registration.

Please, keep in mind that I care about your needs. It is just that tweaking your project's manifest file is a better solution in this case.

UUID: ebbc4602-91c8-4f2e-9b89-acc792d9cec0
Repo: https://github.com/aminya/EzXMLPatched.jl.git
Tree: 630253d31026489ee4911575690c1bb3a9cfead6

Registrator tree SHA: f50e50c1d2a1b9694b1d5749fdb25fef2ca4c291
@JuliaRegistrator JuliaRegistrator force-pushed the registrator/ezxmlpatched/ebbc4602/v1.0.0 branch from 7b8f2e6 to 95134c3 Compare March 1, 2020 05:33
JuliaRegistrator referenced this pull request in aminya/EzXMLPatched.jl Mar 1, 2020
@aminya
Copy link
Contributor

aminya commented Mar 1, 2020

In my opinion, this is the case for a fork: just fork the package, patch it as you please, and use it.

Just to expand on this answer: You can make your own forks of packages (assuming that they have open-source licenses) and use your forks without needing to register your forks or maintain a registry. Simply use the Manifest.toml file to specify the forks and branches that you want to use, and then check your Manifest.toml file into source control. (You will need to make sure that you do not have any entries in your .gitignore file that match Manifest.toml.)

For example, you can take a look at what Keno has done with the manifest for his unregistered package ToyFHE.jl:

* Project file: [JuliaComputing/ToyFHE.jl:Project.toml@`70acf5d`](https://github.com/JuliaComputing/ToyFHE.jl/blob/70acf5d5c14fc9249091e7cd07b57a0123ff8b7f/Project.toml)

* Manifest file: [JuliaComputing/ToyFHE.jl:Manifest.toml@`70acf5d`](https://github.com/JuliaComputing/ToyFHE.jl/blob/70acf5d5c14fc9249091e7cd07b57a0123ff8b7f/Manifest.toml)

As you can see in the manifest file, Keno has forked several of the repos, and ToyFHE.jl uses Keno's forks instead of the upstream repos. As one example, instead of using the upstream BitIntegers.jl repo (rfourquet/BitIntegers.jl), the manifest uses the kf/div branch inside Keno's fork (Keno/BitIntegers.jl).

Of course, you should not create or edit Manifest.toml files by hand. So doing something like ]add https://github.com/Keno/BitIntegers.jl#kf/div in the Pkg REPL mode is how you tell Pkg to use your fork and your branch.

Please, keep in mind that I care about your needs. It is just that tweaking your project's manifest file is a better solution in this case.

But Manifest isn't the same for different Julia versions. Especially, since EzXML uses jil for Julia 1.3.

I am more than happy to get the original repository fixed, but I got forced to do this after having no luck in merging the PR, or overriding the method without warnings.

@DilumAluthge
Copy link
Member

DilumAluthge commented Mar 1, 2020

But Manifest isn't the same for different Julia versions.

For what it is worth, I think that people should be allowed to check in to source control different Manifest.toml files for each version of Julia. So, for example, if your package supports Julia 1.0 and later, you might use each different version of Julia to generate a different Manifest.toml file as follows:

  • Julia 1.0 -> Manifest.1.0.toml
  • Julia 1.1 -> Manifest.1.1.toml
  • Julia 1.2 -> Manifest.1.2.toml
  • Julia 1.3 -> Manifest.1.3.toml

You would check in all of those manifest files (Manifest.1.0.toml, Manifest.1.1.toml , Manifest.1.2.toml, and Manifest.1.3.toml) into source control.

And then, if you want to e.g. load your package on Julia 1.2, you would do this:

cp Manifest.1.2.toml Manifest.toml

And then you would instantiate the manifest.

Disclaimer: There are some people that agree with this approach, and some people that disagree. I don't know if we should necessarily encourage or advocate for this approach. But we should at minimum allow it and have support for it.

@DilumAluthge
Copy link
Member

DilumAluthge commented Mar 1, 2020

Especially, since EzXML uses jil for Julia 1.3.

If the biggest bifurcation is between Julia >= 1.3 and Julia < 1.3, then you might be able to get away with only generating two manifests:

  • Julia 1.0 -> Manifest.1.0.toml
  • Julia 1.3 -> Manifest.1.3.toml

You would check Manifest.1.0.toml and Manifest.1.3.toml into source control. Then, on Julia < 1.3, you would do this:

cp Manifest.1.0.toml Manifest.toml

On Julia >= 1.3, you would instead do this:

cp Manifest.1.3.toml Manifest.toml

@aminya
Copy link
Contributor

aminya commented Mar 1, 2020

Especially, since EzXML uses jil for Julia 1.3.

If the biggest bifurcation is between Julia >= 1.3 and Julia < 1.3, then you might be able to get away with only generating two manifests:

  • Julia 1.0 -> Manifest.1.0.toml
  • Julia 1.3 -> Manifest.1.3.toml

You would check Manifest.1.0.toml and Manifest.1.3.toml into source control. Then, on Julia < 1.3, you would cp Manifest.1.0.toml Manifest.toml. On Julia >= 1.3, you would cp Manifest.1.3.toml Manifest.toml.

Thanks a lot for the answer. I will try this.

@bicycle1885
Copy link

Sometimes I'm too busy to maintain all my packages. That's why I've transferred my five packages to JuliaIO (e.g. https://github.com/JuliaIO/TranscodingStreams.jl) following Stefan's invitation. In fact, I do not check issues and pull requests so often, but I do check my email box so please feel fee to ping me or directly email me when you think your pull requests are overlooked.

@KristofferC
Copy link
Member

KristofferC commented Mar 1, 2020

There is no need at this point to register a fork since it is explicitly stated that is only for internal use in a package and that has better solutions.

This is meant for the internal use of AcuteML.jl, you are not forced to use EzXMLPatched.jl. If Pkg could allow me to include a fork inside the Project.toml, I didn't need registering anymore.

Just copy paste the module into your package. Look at the ext folder in Pkg for an example.

@KristofferC
Copy link
Member

Alternatively, put

Base.unsigned(x::Ptr) = UInt(x)
Base.signed(x::Ptr) = Int(x)

in your package on 1.4 to fix the Julia bug, and the package is automatically fixed. I will close this since it seems like there are better workarounds for the problem at hand.

@KristofferC KristofferC closed this Mar 1, 2020
@DilumAluthge DilumAluthge deleted the registrator/ezxmlpatched/ebbc4602/v1.0.0 branch March 1, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants