Skip to content

Copy test data files to the binary directory#8460

Closed
duanmeng wants to merge 3 commits intofacebookincubator:mainfrom
duanmeng:cpy
Closed

Copy test data files to the binary directory#8460
duanmeng wants to merge 3 commits intofacebookincubator:mainfrom
duanmeng:cpy

Conversation

@duanmeng
Copy link
Collaborator

ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

@netlify
Copy link

netlify bot commented Jan 20, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit d9066db
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65c1e090742178000825f5cd

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 20, 2024
@kgpai kgpai requested a review from assignUser February 1, 2024 05:35
# See the License for the specific language governing permissions and
# limitations under the License.

add_custom_target(example_file_dir ALL)
Copy link
Contributor

Choose a reason for hiding this comment

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

I get what you are doing here, but I reckon this must be a common enough pattern that there must be a simpler , idiomatic way of doing this.

cc : @assignUser

@kgpai
Copy link
Contributor

kgpai commented Feb 1, 2024

@duanmeng I reckon your original implementation was more idiomatic (or correct) -
file(COPY assets DESTINATION ${CMAKE_BINARY_DIR})

@assignUser let us know what you think.

Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

I don't have a lot of bw at the moment but reading up on this a bit I am pretty sure that the custom command just implements the same features that file(COPY) gives us as that will also only be executed if the files/dir don't exist or are older. And it is automatically using relative paths.

The only difference would probably be when installing targets (which we don't do yet) where the custom target would work but file(COPY) would not. But I am not entirely sure about that but can't look into it further at the moment. Though it is unlikely people will run tests outside of the build tree (I don't see us installing tests onto the system?)

So file(COPY) as the more concise version is probably the better choice for now/this case.

I do appreciate the contribution, great to see! Thanks @duanmeng

@duanmeng
Copy link
Collaborator Author

duanmeng commented Feb 6, 2024

So file(COPY) as the more concise version is probably the better choice for now/this case.

@assignUser Thanks for your help. I've gone back to the file(COPY) way according to your and @kgpai advice :)

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in db109f2.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit db109f2f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 9, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 9, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 10, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 10, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 12, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 12, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 12, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
FelixYBW pushed a commit to FelixYBW/velox that referenced this pull request Feb 12, 2024
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
makagonov pushed a commit to makagonov/velox that referenced this pull request May 27, 2025
Summary:
ParquetReaderTest depends on the test data files in directory examples.
It is nice to copy it into the binary directory during the CMake project loading.

Pull Request resolved: facebookincubator#8460

Reviewed By: pedroerp

Differential Revision: D53482409

Pulled By: kgpai

fbshipit-source-id: 45a6be7b041f486bf76344f166d140350345bf63
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants