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

SPARK-5143 [BUILD] [WIP] spark-network-yarn 2.11 depends on spark-network-shuffle 2.10 #4876

Closed
wants to merge 3 commits into from

Conversation

srowen
Copy link
Member

@srowen srowen commented Mar 3, 2015

Update <scala.binary.version> prop in POM when switching between Scala 2.10/2.11

@ScrapCodes for review. This sed command is supposed to just replace the first occurrence, but it replaces them all. Are you more of a sed wizard than I? It may be a GNU/BSD thing that is throwing me off. Really, just the first instance should be replaced, hence the [WIP].

NB on OS X the original sed command here will create files like pom.xml-e through the source tree though it otherwise works. It's like -e is also the arg to -i. I couldn't get rid of that even with -i"". No biggie.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28234 has started for PR 4876 at commit 703e1eb.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28234 has finished for PR 4876 at commit 703e1eb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28234/
Test PASSed.

| xargs -I {} sed -i -e 's|\(artifactId.*\)_2.11|\1_2.10|g' {}

# Also update <scala.binary.version> in parent POM
sed -i -e 's|<scala\.binary\.version>2.11<|<scala.binary.version>2.10<|1' pom.xml
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but can't this be done through profiles in maven ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is how the POMs will be read by tools that just download the POMs to understand dependencies. They read the 'default' config. The alternative would be to edit the file to move <activeByDefault> to the other Scala profile, though that seemed harder.

@ScrapCodes
Copy link
Member

I am not a sed wizard, was just going through the man page. It does not say anything about -e being POSIX or non POSIX - sadly!. I do not have a mac, so no clues about the problem you stated.

@srowen
Copy link
Member Author

srowen commented Mar 4, 2015

Don't worry about -e, I think. Does the command work to replace just the first occurrence on GNU sed? I can dig out a linux box but just wondered if you knew already. also, what's with | instead of /?

@SparkQA
Copy link

SparkQA commented Mar 4, 2015

Test build #28261 has started for PR 4876 at commit e875d4a.

  • This patch merges cleanly.

@srowen
Copy link
Member Author

srowen commented Mar 4, 2015

@ScrapCodes I updated the script to use more 'standard' sed syntax. The new command now works on GNU sed. Neither quite works on BSD sed (a la OS X) so I added a note about that, at least. I don't know how to fully fix that, and given this probably blocks 1.3, wanted to narrowly fix the immediate problem. WDYT?

@SparkQA
Copy link

SparkQA commented Mar 4, 2015

Test build #28262 has started for PR 4876 at commit b060c44.

  • This patch merges cleanly.

@ScrapCodes
Copy link
Member

what's with | instead of /?

sed let's you use any separator of your choice, I can not link you to documentation which validates this claim right away. I am not sure about its POSIX compliance either, (I can look it up in free time though.). For now, I don't see any harm in changing '|' '/' character.

removing the 'g' flag at the end ensure sed only replaces once.

@ScrapCodes
Copy link
Member

I did not run the script personally, but from the code it looks okay(as in harmless) to me. Sorry, I am not yet clear on how useful the addition of the last line is going to be. Because from your comment it is not clear

  1. what tools ?
  2. How useful are those tools themselves ?
    And how does the tool support block 1.3 release ?

@srowen
Copy link
Member Author

srowen commented Mar 4, 2015

@ScrapCodes Right now, the published POMs are incorrect for Scala 2.11, because the parent POM defines scala.binary.version as 2.10, and child modules use that value to describe their dependencies. So the issue in this JIRA arises.

Of course, the POM has a profile to switch this value for Scala 2.11. But this has no effect on how anyone consuming these POMs parse the POMs. They just read the default value instead that we've published for Scala 2.11. That is, when any tool (Maven etc) reads these POMs, they do so as if without any special flags, vars, profiles activated.

So we also have to edit this value manually when making the 2.11 POM.

It's not a problem with tools; the dependencies described by the Spark Scala-2.11 POMs are wrong as a result of this, and any tool that reads them correctly reads the incorrect dependencies.

@SparkQA
Copy link

SparkQA commented Mar 4, 2015

Test build #28261 has finished for PR 4876 at commit e875d4a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28261/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Mar 4, 2015

Test build #28262 has finished for PR 4876 at commit b060c44.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28262/
Test PASSed.

@ScrapCodes
Copy link
Member

Hmm.. I was trusting our effective pom trick was working properly. Forgot
the JIRA id for that though.
On Mar 4, 2015 6:10 PM, "Sean Owen" [email protected] wrote:

@ScrapCodes https://github.com/ScrapCodes Right now, the published POMs
are incorrect for Scala 2.11, because the parent POM defines
scala.binary.version as 2.10, and child modules use that value to
describe their dependencies. So the issue in this JIRA arises.

Of course, the POM has a profile to switch this value for Scala 2.11. But
this has no effect on how anyone consuming these POMs parse the POMs. They
just read the default value instead that we've published for Scala 2.11.
That is, when any tool (Maven etc) reads these POMs, they do so as if
without any special flags, vars, profiles activated.

So we also have to edit this value manually when making the 2.11 POM.

It's not a problem with tools; the dependencies described by the Spark
Scala-2.11 POMs are wrong as a result of this, and any tool that reads them
correctly reads the incorrect dependencies.


Reply to this email directly or view it on GitHub
#4876 (comment).

@ScrapCodes
Copy link
Member

Based on your comments and this issue I am going to take a deeper look tomorrow again. Meanwhile if you are rushed with respect to this PR. Looks like a good immediate solution.

@pwendell
Copy link
Contributor

pwendell commented Mar 4, 2015

I commented on the JIRA. This LGTM as an immediate fix - clearly the property is not correct in the published 2.11 poms.

asfgit pushed a commit that referenced this pull request Mar 5, 2015
…work-shuffle 2.10

Update `<scala.binary.version>` prop in POM when switching between Scala 2.10/2.11

ScrapCodes for review. This `sed` command is supposed to just replace the first occurrence, but it replaces them all. Are you more of a `sed` wizard than I? It may be a GNU/BSD thing that is throwing me off. Really, just the first instance should be replaced, hence the `[WIP]`.

NB on OS X the original `sed` command here will create files like `pom.xml-e` through the source tree though it otherwise works. It's like `-e` is also the arg to `-i`. I couldn't get rid of that even with `-i""`. No biggie.

Author: Sean Owen <[email protected]>

Closes #4876 from srowen/SPARK-5143 and squashes the following commits:

b060c44 [Sean Owen] Oops, fixed reversed version numbers!
e875d4a [Sean Owen] Add note about non-GNU sed; fix new pom.xml update to work as intended on GNU sed
703e1eb [Sean Owen] Update scala.binary.version prop in POM when switching between Scala 2.10/2.11

(cherry picked from commit 7ac072f)
Signed-off-by: Patrick Wendell <[email protected]>
@asfgit asfgit closed this in 7ac072f Mar 5, 2015
@srowen srowen deleted the SPARK-5143 branch March 7, 2015 00:47
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