-
Notifications
You must be signed in to change notification settings - Fork 240
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
Added in profile to enable shims for SNAPSHOT releases #611
Conversation
Signed-off-by: Robert (Bobby) Evans <[email protected]>
build |
so to clarify, this is active by default but if you specify another profile it disables this one? I would think we want this profile specified for everything except the release build. |
Yes, specifying any other profile explicitly on the command-line will disable the snapshot-shims profile.
The problem with that setup is we would never be regularly testing what we're planning to ship. From a cursory glance, it appears the premerge CI is specifying some profiles which would turn this off ( Also how would this interact with the spark*tests profiles? If someone specifies the |
I agree that we should probably be specific about all of the profiles for the builds. I missed the
From my experience with maven there is no way to have one profile activate/deactivate another profile. We could, however, have a property that activates multiple profiles. So they would both be independent of each other. |
+1 for this. That way we can ask for spark versions to test against with property settings, and those will enable both the test profile for a spark version along with the shim if that shim version happens to be a snapshot. |
I would have to do some testing to see if we can make it work mixing properties and explicit setting of profiles. |
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, would be good to hear from @tgravescs.
jenkins/spark-nightly-build.sh
Outdated
@@ -19,11 +19,11 @@ set -ex | |||
|
|||
. jenkins/version-def.sh | |||
|
|||
mvn -U -B -Pinclude-databricks clean deploy $MVN_URM_MIRROR -Dmaven.repo.local=$WORKSPACE/.m2 | |||
mvn -U -B '-Pinclude-databricks,!snapshot-shims' clean deploy $MVN_URM_MIRROR -Dmaven.repo.local=$WORKSPACE/.m2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will deploy the nightly version without the snapshot shims to urm. Which means devs and integration tests can't use it. Our integration test builds rely on this now.
if we want to keep integration testing nightly what we actually push, then we would need 2 separate jars - one with shims and one without, otherwise we aren't integration testing the shims which I think is a bad idea. otherwise we push the version with all the shims and then just test the one without shims differently. This doesn't seem high risk to me to have to test it on every premerge, I would rather just test nightly or something before a release. |
build |
@tgravescs I addressed your comments on the nightly deploy. |
* Added in profile to enable shims for SNAPSHOT releases Signed-off-by: Robert (Bobby) Evans <[email protected]> * Updated jenkins files to be explicit about snapshot-shims * Addressed review comments
* Added in profile to enable shims for SNAPSHOT releases Signed-off-by: Robert (Bobby) Evans <[email protected]> * Updated jenkins files to be explicit about snapshot-shims * Addressed review comments
…VIDIA#611) * fixing incorrect invalidation of positive cudf scale decimal values * signoff Signed-off-by: Mike Wilson <[email protected]> * typo fix Signed-off-by: Mike Wilson <[email protected]> * Update src/main/cpp/src/cast_string.cu Co-authored-by: Nghia Truong <[email protected]> Signed-off-by: Mike Wilson <[email protected]> Co-authored-by: Nghia Truong <[email protected]>
This fixes #602
I didn't update any of the Jenkins files because they either already use a profile for databricks that would disable the
snapshot-shims
profile or they have it enabled by default. I could update the premerge to try and have this off by default, but I would need to add a second one that has it enabled too, so we know if we are missing something.