osdir.com

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: (fyi) PR#7324 updates how projects consume vendored guava




On Thu, Dec 20, 2018 at 5:29 AM Ismaël Mejía <iemejia@xxxxxxxxx> wrote:
I may be misreading or probably missed part of the previous
discussion, but why don't we have the vendoring artifacts in a
different repo?
This will do the release really independent and avoid the artificial
staging step, no?

Other than convenience, I believe there is just one technical reason: GrpcVendoring [1] contains the logic where we map quite a few packages into the org.apache.beam.vendor.grpc.v1p13p1. namespace. This logic is used by:

1. vendored gRPC: to produce the vendored artifact [2]
2. `applyPortabilityNature` [3] [4] because we have to first generate Java code from the proto files and _then_ do the relocation.

gRPC is a special case; no other vendored artifact is expected to have this kind of sharing

 
Thomas what do you mean with:
> It is unfortunate that we have to change the imports on every patch. Is it not sufficient to just update the artifact version?
That we should not be adding the version to the vendored Java
packages? do you have some idea on how to fix this?

We put the full version string into Java package name so it cannot conflict. I think many choices work here. Luke summarized pro/con of different options very well when we made this decision: https://lists.apache.org/thread.html/6bc90677d6936f59676a68f65d810d723ddac27022209a90a04c1df5@%3Cdev.beam.apache.org%3E

I don't favor re-opening that discussion yet, until we have some more experience. I'd rather push forward to gain more benefits from what we have done. It is very easy to change our minds: we can always publish a different vendored artifact series and do automatic refactor to cut off the version from the imports.

Kenn


 



On Thu, Dec 20, 2018 at 12:00 AM Scott Wegner <swegner@xxxxxxxxxx> wrote:
>
> > Can testing of the artifact prior to release not be done against the staging repo?
>
> I suspect it could be. The process for making these updates has not yet been defined, and I agree that having this intermediate state on every update is not ideal.
>
> (also, I mistakenly wrote Guava in the above email; this fix was actually for gRPC)
>
> Here's a proposal for a path forward:
>
> 1. Release a new vendored guava artifact
> 2. Switch all Beam projects to consume the vendored artifact from Maven Central
>   2b. Add validation to ensure all projects reference guava consistently from Maven Central and not the build.
>
> And then define the process for future vendoring updates:
> 3. Increment the vendored artifact version number, and make changes to the vendored source project
> 4. Push the new vendored artifact to a staging repository
> 5. In a separate commit, add the staging repository as a dependency source
> 6. Increment the vendored artifact dependency to the new version and update consuming code as necessary
> 7. Open a PR with all changes for validation / code review, but don't merge yet
> 8. Open a separate PR with just the vendored artifact changes (up to step 4)
> 9. Once reviewed/merged, kick off a release of the vendored artifact
> 10. Once released, remove staging repository for pending PR (undo step 5), and merge after reviewed.
>
>
> This process would be more tedious / time-consuming, but less disrupting for other contributors.
>
> On Wed, Dec 19, 2018 at 2:06 PM Thomas Weise <thw@xxxxxxxxxx> wrote:
>>
>> Hi Scott,
>>
>> Can testing of the artifact prior to release not be done against the staging repo?
>>
>> See here for an example: https://github.com/apache/beam/pull/7322/files
>>
>> It is unfortunate that we have to change the imports on every patch. Is it not sufficient to just update the artifact version?
>>
>> Thomas
>>
>>
>>
>> On Wed, Dec 19, 2018 at 1:49 PM Scott Wegner <scott@xxxxxxxxxx> wrote:
>>>
>>> Just a heads up: PR#7324 [1] just got merged which fixes an issue with our vendored guava artifact and JNI: see BEAM-6056 [2] for details. Important changes for you:
>>>
>>> a. IntelliJ doesn't understand how our vendored guava is built and will give red squigglies. This is annoying but temporary while we release a new vendored artifact.
>>> b. The guava vendoring prefix has changed. If you have work-in-progress, you'll hit this when you merge into master. Update any new guava imports to: org.apache.beam.vendor.grpc.v1p3p1.[..]
>>>
>>> Read on for details..
>>>
>>> The fix for BEAM-6056 included two changes:
>>> 1. Updated package prefix for vendored guava symbols: o.a.b.vendor.grpc.v1_13_1 -> o.a.b.vendor.grpc.v1p3p1
>>> 2. Move project dependencies to consume vendored guava from :beam-vendor-grpc-1_13_1 project rather than the published Maven artifact.
>>>
>>> Consuming from the integrated project artifact (#2) was necessary in order to make the changes and validate them inline without needing to first publish a Maven release. However, this has some downsides: notably, IntelliJ struggles to understand shaded dependencies [3]. I believe we should move back to consuming pre-published vendor artifacts. The first step is to publish a new vendored beam-vendor-grpc-1_13_1 artifact; I can start that process.
>>>
>>>
>>> [1] https://github.com/apache/beam/pull/7324
>>> [2] https://issues.apache.org/jira/browse/BEAM-6056
>>> [3] https://lists.apache.org/thread.html/4c12db35b40a6d56e170cd6fc8bb0ac4c43a99aa3cb7dbae54176815@%3Cdev.beam.apache.org%3E
>>>
>>> Got feedback? tinyurl.com/swegner-feedback
>
>
>
> --
>
>
>
>
> Got feedback? tinyurl.com/swegner-feedback