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

[core] Fix ycsb.sh and ycsb.bat missing core dependencies #908

Merged

Conversation

risdenk
Copy link
Collaborator

@risdenk risdenk commented Jan 19, 2017

Fixes #877.

I don't know if this is the best way to solve this. Basically the core module's parent was root before. I made it binding parent so that it will pickup the necessary assembly plugin and create the lib directory. I'm not really familiar with Maven module parents and dependencies.

@risdenk risdenk added this to the 0.13.0 milestone Jan 19, 2017
@risdenk risdenk self-assigned this Jan 19, 2017
@risdenk risdenk requested a review from busbey January 19, 2017 19:39
@risdenk
Copy link
Collaborator Author

risdenk commented Feb 3, 2017

@busbey - Sorry to keep bugging you with reviews. Not sure who else would know how this should work. If there is someone else to ping let me know.

@busbey
Copy link
Collaborator

busbey commented May 12, 2017

sorry I lost track of this one.

@busbey - Sorry to keep bugging you with reviews. Not sure who else would know how this should work. If there is someone else to ping let me know.

Might be time to bring in some additional project maintainers.

I don't think adding binding-parent to be the core's parent is the right way to do this. I'm trying to grok the underlying problem reading a few of the referenced problems.

Is it just when using a source checkout and a binding other than the built-in dummy one?

@dotnwat
Copy link

dotnwat commented May 20, 2017

I'm seeing this issue with a source checkout.

@dotnwat
Copy link

dotnwat commented May 20, 2017

I added something like this to ycsb.sh where the class path is constructed for the source case and it seems to work. not sure what the best thing is.

+  # Core dependencies from Maven
+  CLASSPATH_FILE=$(mktemp)
+  mvn -pl com.yahoo.ycsb:core dependency:build-classpath -Dmdep.outputFile=$CLASSPATH_FILE
+  CLASSPATH_MVN=$(cat $CLASSPATH_FILE)
+  echo XXX $CLASSPATH_MVN
+  CLASSPATH=$CLASSPATH:$CLASSPATH_MVN
 fi

Copy link
Collaborator

@busbey busbey left a comment

Choose a reason for hiding this comment

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

I prefer noahdesu's approach.

@risdenk risdenk force-pushed the fix_core_dependencies_source_checkout branch 3 times, most recently from fe138f9 to 55311a7 Compare September 16, 2017 21:12
@risdenk
Copy link
Collaborator Author

risdenk commented Sep 16, 2017

@busbey so I had a second look at this. It is simpler to copy the dependencies for core and slightly modify the ycsb.sh and yscb.bat to pick up those dependencies. The parsing of mvn dependency:build-classpath isn't easy to do with Windows.

Copy link
Collaborator

@busbey busbey left a comment

Choose a reason for hiding this comment

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

That sounds much better!

Could we put the copy into a profile that we activate when we do the source build from the shell scripts? That way folks doing normal maven builds won't have a spurious jar copy.

@risdenk
Copy link
Collaborator Author

risdenk commented Sep 17, 2017

@busbey yea that sounds reasonable. I'll update the PR.

@manolama
Copy link
Collaborator

@risdenk Still want this in v13?

@manolama manolama modified the milestones: 0.13.0, 0.14.0 Sep 19, 2017
@risdenk risdenk force-pushed the fix_core_dependencies_source_checkout branch from 55311a7 to 61c646c Compare September 20, 2017 00:29
@risdenk
Copy link
Collaborator Author

risdenk commented Sep 20, 2017

@manolama up to you. I'm done making changes now unless there are required changes from reviews.

@busbey Can you take a look at the latest changes? I don't have a Windows machine to test on right now. It works on my Mac. I switched from for to ls since it was easier to read and shouldn't be too many files in the directory that are *.jar.

Copy link
Collaborator

@busbey busbey left a comment

Choose a reason for hiding this comment

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

these look reasonable. if you'd like me to test on a windows machine, I can probably do that by the end of the week.

@risdenk
Copy link
Collaborator Author

risdenk commented Sep 20, 2017

@busbey that would be great. The tests I ran:

mvn clean
bin/ycsb.sh load basic -P workloads/workloada
bin/ycsb.sh load rest -P workloads/workloada

This will make sure that basic works and builds correctly. Then check that another binding runs without getting a classpath error.

@busbey
Copy link
Collaborator

busbey commented May 18, 2018

confirmed this didn't get any classpath trouble on Windows 10 Home

git clean -xdf
.\bin\ycsb.bat load basic -P .\workloads\workloada
.\bin\ycsb.bat load rest -P .\workloads\workloada

@busbey busbey merged commit 3b6059b into brianfrankcooper:master May 18, 2018
@busbey busbey mentioned this pull request May 23, 2018
@busbey busbey mentioned this pull request Jun 7, 2019
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.

4 participants