accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubbsii <...@git.apache.org>
Subject [GitHub] accumulo pull request #131: ACCUMULO-4356 Remove bundled jars from -bin.tar....
Date Fri, 22 Jul 2016 20:08:09 GMT
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/131#discussion_r71937940
  
    --- Diff: assemble/src/main/scripts/generate-download-script.sh ---
    @@ -0,0 +1,56 @@
    +#! /usr/bin/env bash
    +
    +# Licensed to the Apache Software Foundation (ASF) under one or more
    +# contributor license agreements.  See the NOTICE file distributed with
    +# this work for additional information regarding copyright ownership.
    +# The ASF licenses this file to You under the Apache License, Version 2.0
    +# (the "License"); you may not use this file except in compliance with
    +# the License.  You may obtain a copy of the License at
    +#
    +#     http://www.apache.org/licenses/LICENSE-2.0
    +#
    +# Unless required by applicable law or agreed to in writing, software
    +# distributed under the License is distributed on an "AS IS" BASIS,
    +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +# See the License for the specific language governing permissions and
    +# limitations under the License.
    +
    +# This script will generate a DEPENDENCIES listing of packaged dependencies
    +
    +in=target/dependencies.raw.txt
    +out=target/download-dependencies
    +
    +cat >"$out" <<'EOF'
    +#! /usr/bin/env bash
    +# This script downloads the following jars, identified by their maven
    +# coordinates, using the maven-dependency-plugin.
    +#
    +# DISCLAIMER: This is only one possible way to download a set of dependencies
    --- End diff --
    
    > Long term, perhaps.
    
    I'm trying to look for long-term benefits, yes, at the cost of some short-term road bumps.
I am also trying to make those short-term road bumps minimal, though... hence the inclusion
of this downloader script to ease people along. If I could make this particular road bump
even smaller, I'd do it. I'm not sure how much smaller it can get, though.
    
    > It doesn't invalidate the ability for downstream people to still do whatever they
want.
    
    True, but I think it's a bad thing to do by default, and is going to encourage bad practice.
Relocating is also another word for a certain kind of "forking". By relocating/forking the
code into our own, we're taking on the future responsibility of those dependencies, rather
than deferring to their respective communities. I think that's not something we should do.
    
    > These classes should also not be getting dynamically loaded, should they
    
    Depends on what's loading what. We may not have any dynamically loaded dependencies on
these particular libraries, but we easily could in the future (for example, to support both
jline1 and jline2), and we have no idea what the dependencies themselves are doing which would
be affected by relocation.
    
    > It would simplify the case for users who just want to get up and running (that are
not release engineering experts).
    
    I'm not convinced that relocation would help users get going out-of-box, and certainly
not significantly over running a simple downloader script to gather dependencies, and I think
this would be a short-sighted change favoring small short-term benefits over the long-term
health of the project. Depending on how it's done (automated, or copy/pasted/renamed), it
could also add a lot of technical debt to Accumulo, locking it in to using code which diverges
from our dependencies' patched upstreams.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message