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.

Sign up for GitHub

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

Jump to bottom

HPCC-29432 rebase and fix issues dependency/package issues for 9.4.x #17806

Closed
wants to merge 1 commit into from
Closed

HPCC-29432 rebase and fix issues dependency/package issues for 9.4.x #17806

wants to merge 1 commit into from

Conversation

Michael-Gardner
Copy link
Contributor

@Michael-Gardner Michael-Gardner commented Sep 22, 2023 •
edited
Loading

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@Michael-Gardner
Copy link
Contributor Author

@GordonSmith I did a complete rework of this branch. Included some tweaks to the NLP plugin addition you did a few months ago in order to ensure extra files weren't getting added to each plugin package.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
.github/workflows/build-assets.yml
@@ -185,7 +185,7 @@ jobs:
run: |
mkdir -p ${{ needs.preamble.outputs.folder_build }}
echo "${{ secrets.SIGNING_SECRET }}" > ${{ needs.preamble.outputs.folder_build }}/private.key
plugins=("CASSANDRAEMBED" "COUCHBASEEMBED" "ECLBLAS" "H3" "JAVAEMBED" "KAFKA" "MEMCACHED" "MONGODBEMBED" "MYSQLEMBED" "NLP" "REDIS" "REMBED" "SQLITE3EMBED" "SQS" "PLATFORM" "CLIENTTOOLS_ONLY")
plugins=("COUCHBASEEMBED" "H3" "JAVAEMBED" "KAFKA" "MEMCACHED" "MONGODBEMBED" "MYSQLEMBED" "NLP" "REDIS" "REMBED" "SQLITE3EMBED" "SQS" "PLATFORM" "CLIENTTOOLS_ONLY")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cassanda and ECLBlas are both included in the base platform package. This just removes the option of building a 'plugin' for them too and confusing users.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
CMakeLists.txt
HPCC_ADD_SUBDIRECTORY (system/hrpc "NLP")
HPCC_ADD_SUBDIRECTORY (system/mp "NLP")
HPCC_ADD_SUBDIRECTORY (system/security/securesocket "NLP")
HPCC_ADD_SUBDIRECTORY (system/security/zcrypt "NLP")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These line changes were made to keep compile times down when we are building plugins. Those directories weren't needed to create the other plugins. If it's an NLP package, they'll get included in the build. But I went through those directories and added guards on the install() functions so we don't add a bunch of libraries to NLP that are already in the base platform.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
CMakeLists.txt
@@ -218,7 +218,7 @@ if(APPLE)
HPCC_ADD_SUBDIRECTORY(package)
endif(APPLE)

if (NOT CLIENTTOOLS_ONLY)
if (PLATFORM)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes all the vcpkg include files from getting added to each plugin package.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
cmake_modules/options.cmake
@@ -9,6 +9,7 @@ option(DEVEL "Enable the building/inclusion of a Development component." OFF)
option(CLIENTTOOLS_ONLY "Enable the building of Client Tools only." OFF)
option(INCLUDE_PLUGINS "Enable the building of platform and all plugins for testing purposes" OFF)
option(USE_CASSANDRA "Include the Cassandra plugin in the base package" ON)
option(USE_ECLBLAS "Include the ECLBLAS plugin in the base package" ON)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eclblas was included in the base package already, so I made it work similar to cassandra so there's no confusion in the future.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
cmake_modules/plugins.cmake
@@ -70,6 +68,10 @@ if (USE_CASSANDRA)
set(VCPKG_CASSANDRAEMBED "${VCPKG_INCLUDE}")
endif()

if (USE_ECLBLAS)
set(VCPKG_ECLBLAS "${VCPKG_INCLUDE}")
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this block was necessary for the vcpkg.json file to get interpreted correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Now that ECL Blas is not a plugin, it will need to be manually set to VCPKG_SUPRESS, similar to what we do for VCPKG_APR below

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
common/deftype/CMakeLists.txt
TARGETS deftype
RUNTIME DESTINATION ${EXEC_DIR}
LIBRARY DESTINATION ${LIB_DIR})
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block is a guard that I added to several of the subdirectories. All the plugins were adding extra files that weren't needed from these directories because of the additions to the if(PLUGIN) add_subdirectory() portion of the top level cmake when we added NLP.

This mimics the behavior we have in jlib already, and a few other directories we were building to support plugin development.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
plugins/couchbase/CMakeLists.txt
install(
TARGETS couchbaseembed
RUNTIME_DEPENDENCY_SET couchbaseembed_deps
DESTINATION plugins)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CALC_DEPS was grabbing to much. It did a great job of including all the dependencies, unfortunately some of those dependencies are built into the platform by default. So this RUNTIME_DEPENDENCY_SET is a way to have more control over the process. Allows us to find dependencies of under vcpkg_installed, and the PRE_EXCLUDE_REGEXES will sort out any of the linked sonames so we don't even search for them, removing the chance of package conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

How does this handle the dependencies of dependencies issue? (or have you manually checked that list?)

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
plugins/eclblas/CMakeLists.txt
endif()
endif()
HPCC_ADD_LIBRARY(eclblas SHARED ${SRCS})
install ( TARGETS eclblas RUNTIME DESTINATION ${EXEC_DIR} LIBRARY DESTINATION ${LIB_DIR} CALC_DEPS) # No need to put in plugins dir
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CALC_DEPS is fine here since eclblas is included in the platform. Fixed some indentation and swapped to USE_ECLBLAS since this isn't treated like a plugin.

Michael-Gardner
Michael-Gardner commented Sep 22, 2023
View reviewed changes
plugins/sqs/CMakeLists.txt
DESTINATION plugins
CALC_DEPS
)
if(WIN32 OR APPLE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't heard of this package file conflict being an issue on macs. I kept this logic exactly the same for any build other than linux.

GordonSmith
GordonSmith requested changes Sep 26, 2023
View reviewed changes
cmake_modules/plugins.cmake
@@ -70,6 +68,10 @@ if (USE_CASSANDRA)
set(VCPKG_CASSANDRAEMBED "${VCPKG_INCLUDE}")
endif()

if (USE_ECLBLAS)
set(VCPKG_ECLBLAS "${VCPKG_INCLUDE}")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Now that ECL Blas is not a plugin, it will need to be manually set to VCPKG_SUPRESS, similar to what we do for VCPKG_APR below

plugins/couchbase/CMakeLists.txt
install(
TARGETS couchbaseembed
RUNTIME_DEPENDENCY_SET couchbaseembed_deps
DESTINATION plugins)
Copy link
Member

Choose a reason for hiding this comment

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

How does this handle the dependencies of dependencies issue? (or have you manually checked that list?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GordonSmith GordonSmith GordonSmith requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Michael-Gardner @GordonSmith