Update to ProtoBuf 1.0.11 #124
Conversation
nomadbl
changed the title
|
I don't have the time to maintain the package anymore, you should ping some other maintainers in the org... |
|
@c42f @tanmaykm @quinnj @pfitzseb @fredrikekre |
|
(BTW while you are at it, you should clean up the unit test runners to stop testing this on Julia 1.3, and probably bump the minimum required version to the 1.6 LTS..) |
|
This looks good to me. I believe I have admin on the org and have been included in this package from the beginning. I will try and give this a proper review early next week. |
| src_dir = cur_path/"proto" | ||
| out_dir = cur_path/"protojl" | ||
| # src_dir = cur_path/"proto" | ||
| src_dir = PosixPath(".")/"proto" |
There was a problem hiding this comment.
Why was this changed?
Doesn't this break things in windows?
There was a problem hiding this comment.
Since this only runs once when building the package (specifically, when updating ProtoBuf) I don't think this should pose a problem. In particular there is nothing windows specific here (the generated files are commited and this has no reason to run under windows).
However if any Unit test fails under windows, then we can see about this.
There was a problem hiding this comment.
In principle it should work either way but ProtoBuf 1.0.0 code generation is annoying, so if at all possible I would like to avoid repeating this.
|
|
||
| # Input files | ||
| infiles = glob("*.proto", src_dir/input_path) | ||
| infiles = split.(string.(glob("*.proto", src_dir/input_path)), '/') .|> (a -> a[3:end]) .|> a -> joinpath(a...) |
There was a problem hiding this comment.
For readability can we avoid using |> ?
This inparticular I think is the same as:
| infiles = split.(string.(glob("*.proto", src_dir/input_path)), '/') .|> (a -> a[3:end]) .|> a -> joinpath(a...) | |
| infiles = glob("proto/$(input_path)/*.proto", src_dir/input_path)), '/') |
There was a problem hiding this comment.
In all honesty, this code should probably avoid the use of PosixPath altogether, which I used here in the first place because it was already in use and I tried to change as little as possible.
As it is now, it's hard for me to say if this change will work or not. It worked when run, but fails in the REPL. In the interest of my sanity lets do this in a separate PR (which I'll be happy to help with).
Since this whole gen code only needs to run once, lets leave it for now 🙏
| @@ -11,8 +11,10 @@ ENV["GKSwstype"] = "100" | |||
| ENV["DATADEPS_ALWAYS_ACCEPT"] = true | |||
|
|
|||
| LOG_DIRS = Any["test_logs/"] | |||
| test_s3 = false # Minio breaks on my system | |||
There was a problem hiding this comment.
should this be turned on on CI?
There was a problem hiding this comment.
As the comment suggests, this throws Minio errors on my machine which I was unable to fix, so I disabled it.
If you can run the tests with true here it would be a big help 👍
If it works for you, then sure lets turn that on for CI
Co-authored-by: Frames White <oxinabox@ucc.asn.au>
Co-authored-by: Frames White <oxinabox@ucc.asn.au>
This project is great, which is why I wanted to use it.
The problem was I was getting version conflicts with other libraries due to using a very old version of
ProtoBufSo, this PR does the following:
nothing, adding a bit of typing for clarity..This update may break older Julia builds, so please help me test it as you see fit. I tested with Julia1.9 including running the tests and verifying the examples. Exceptions:
@test_broken. I don't know what the internal graph functions do so please fix that in a following separate PR.All in all, this update will make sure the project does not die when more libraries update.