You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/04/28 09:35:54 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3362: Add CI check to detect Thrift changes
ctubbsii opened a new pull request, #3362:
URL: https://github.com/apache/accumulo/pull/3362
Add a GitHub Actions check that re-generates the Thrift generated code, and compares the alterations to what's committed to the repo. The check will fail if Thrift generates something that isn't checked in. This ensures that we can ignore generated code changes when reviewing pull requests, because if the generated code contained anything unexpected, this check would make it obvious.
This fixes #3095
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3362: Add CI check to detect Thrift changes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3362:
URL: https://github.com/apache/accumulo/pull/3362#issuecomment-1527329173
I'm just glad that the binary I built in Fedora worked on the Ubuntu image... for the first few commits in this PR, I wasn't sure. But it turned out I was just forgetting to `chmod +x` the file. But, I'm glad those first few failed, because the end result is better for having to troubleshoot that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3362: Add CI check to detect Thrift changes
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3362:
URL: https://github.com/apache/accumulo/pull/3362#discussion_r1180449729
##########
contrib/ci/install-thrift.sh:
##########
@@ -0,0 +1,31 @@
+#! /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
+#
+# https://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.
+#
+
+# Download the correct version of Thrift to do CI checks
+
+set -e
+
+thriftdefault="0.16.0"
+rootDir=$(git rev-parse --show-toplevel 2>/dev/null) || ver=$thriftdefault
+ver=$({ xmllint --shell "$rootDir/pom.xml" <<<'xpath /*[local-name()="project"]/*[local-name()="properties"]/*[local-name()="thrift.version"]/text()' | grep content= | cut -f2 -d=; } 2>/dev/null || echo "$thriftdefault")
+ver=${ver%%-*}
+
+sudo wget "https://dist.apache.org/repos/dist/dev/accumulo/devtools/thrift-$ver/thrift" -O /usr/local/bin/thrift &&
Review Comment:
What is this devtools thing on dist? Have not seen this before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3362: Add CI check to detect Thrift changes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3362:
URL: https://github.com/apache/accumulo/pull/3362#issuecomment-1676833358
Backported to 2.1.2 as b59dfa9564da59a3e7dff79dd0220aae45af25bf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3362: Add CI check to detect Thrift changes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3362:
URL: https://github.com/apache/accumulo/pull/3362#discussion_r1180472342
##########
contrib/ci/install-thrift.sh:
##########
@@ -0,0 +1,31 @@
+#! /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
+#
+# https://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.
+#
+
+# Download the correct version of Thrift to do CI checks
+
+set -e
+
+thriftdefault="0.16.0"
+rootDir=$(git rev-parse --show-toplevel 2>/dev/null) || ver=$thriftdefault
+ver=$({ xmllint --shell "$rootDir/pom.xml" <<<'xpath /*[local-name()="project"]/*[local-name()="properties"]/*[local-name()="thrift.version"]/text()' | grep content= | cut -f2 -d=; } 2>/dev/null || echo "$thriftdefault")
+ver=${ver%%-*}
+
+sudo wget "https://dist.apache.org/repos/dist/dev/accumulo/devtools/thrift-$ver/thrift" -O /usr/local/bin/thrift &&
Review Comment:
I just created it and uploaded a thrift binary that I made myself. We have a dist area for releases, and one for development. Lots of projects use the one for development for staging release candidates. We have a different process for that, so we've never used this area for anything. I found a good use for it for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3362: Add CI check to detect Thrift changes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3362:
URL: https://github.com/apache/accumulo/pull/3362#issuecomment-1676832591
Backported to 2.1.2 as b59dfa9564da59a3e7dff79dd0220aae45af25bf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3362: Add CI check to detect Thrift changes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3362:
URL: https://github.com/apache/accumulo/pull/3362
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3362: Add CI check to detect Thrift changes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3362:
URL: https://github.com/apache/accumulo/pull/3362#issuecomment-1527334613
I'm only committing this to main, because I don't expect any thrift changes in earlier branches. But it works the same across all branches... just need to upload a different version of thrift for the different branches. This is actually a variation on what I use locally to resolve the different built versions of Thrift for the different maintenance branches.
If anybody is curious, that script is:
```bash
#! /usr/bin/bash
thriftroot=$HOME/apps/thrift
set -e
thriftdefault="0.16.0"
rootDir=$(git rev-parse --show-toplevel 2>/dev/null ) || ver=$thriftdefault
ver=$({ xmllint --shell "$rootDir/pom.xml" <<<'xpath /*[local-name()="project"]/*[local-name()="properties"]/*[local-name()="thrift.version"]/text()' | grep content= | cut -f2 -d=; } 2>/dev/null || echo "$thriftdefault")
ver=${ver%%-*}
thrift=$thriftroot/thrift-$ver/compiler/cpp/thrift
[[ ! -x $thrift ]] && thrift=$thriftroot/thrift-$thriftdefault/compiler/cpp/thrift
$thrift "$@"
```
I just put this in my local PATH (in `~/bin/thrift`) and I extract copies of thrift releases into `~/apps/thrift` in separate directories. And I build them there, until they fail (they usually fail), but by then the thrift code generator is compiled... and that's all you really need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org