Add common dependencies with include_only

[bhess]

Fixes #2380

Allows upstream algorithms to declare common dependencies that are stored once but compiled into each algorithm variant individually.

How to use

1. Add common_meta_path to the upstream entry in copy_from_upstream.yml:

- name: myupstream
  git_url: https://github.com/...
  sig_meta_path: 'META/{pretty_name_full}_META.yml'
  common_meta_path: 'META/COMMONS.yml'   # path to commons definition
  sig_scheme_path: '.'

2. Create a COMMONS.yml in the upstream repo:

commons:
  - name: myalg_common
    folder_name: .           # root folder of the common sources
    sources: ./src/foo.c ./src/bar.c ./include/params.h
    include_only: true       # compile into each variant, not as a shared OBJECT library

• include_only: false (default): shared sources are compiled once as a standalone OBJECT library and linked into all variants that reference it. Use this when the shared code is truly flag-independent.
• include_only: true: shared sources are copied flat and added directly to each variant's add_library(... OBJECT ...), with the common directory added as target_include_directories. Each variant recompiles the shared sources with its own flags.

3. Reference the common dep from the implementation entry in the upstream META.yml:

implementations:
  - name: opt
    common_dep: myalg_common
  - name: opt_avx2
    common_dep: myalg_common myalg_common_avx2   # space separated

Descriptions and documentation assisted by AI

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged. Also, make sure to update the list of algorithms in the continuous benchmarking files: .github/workflows/kem-bench.yml and sig-bench.yml)

[coveralls]

coverage: 60.288% (-22.0%) from 82.272% — bhe-commondep-inclonly into main

[bhess]

Marking this PR as ready for review.

The logic has been tested in #2361. Additionally, I have included a new README for the copy_from_upstream mechanism to assist future contributors.

[xuganyu96]

Code change looks good to me. Minor suggestion in comments.

[dstebila]

Since #2382 has been merged, can this be closed?

[xuganyu96]

Since #2382 has been merged, can this be closed?

I (locally) rebased this branch on top of main@b0853b2 and found the following diff:

diff --git a/scripts/copy_from_upstream/copy_from_upstream.py b/scripts/copy_from_upstream/copy_from_upstream.py
index 4b25f890b..9010c4665 100755
--- a/scripts/copy_from_upstream/copy_from_upstream.py
+++ b/scripts/copy_from_upstream/copy_from_upstream.py
@@ -333,10 +333,10 @@ def load_instructions(file='copy_from_upstream.yml'):
             scheme['default_implementation'] = family['default_implementation']
             for impl in scheme['metadata']['implementations']:
                 if 'common_dep' in impl:
-                    cdeps_names = impl['common_dep'].split(" ")
+                    impl['common_dep'] = impl['common_dep'].split()
                     sname = scheme['pretty_name_full']
                     uloc = scheme['upstream_location']
-                    for cdep_name in cdeps_names:
+                    for cdep_name in impl['common_dep']:
                         cdep = upstreams[uloc]['commons'][cdep_name]
                         if 'required_flags' in cdep:
                             family['all_required_flags'].update(cdep['required_flags'])
@@ -444,10 +444,10 @@ def load_instructions(file='copy_from_upstream.yml'):
             scheme['default_implementation'] = family['default_implementation']
             for impl in scheme['metadata']['implementations']:
                 if 'common_dep' in impl:
-                    cdeps_names = impl['common_dep'].split(" ")
+                    impl['common_dep'] = impl['common_dep'].split()
                     sname = scheme['pretty_name_full']
                     uloc = scheme['upstream_location']
-                    for cdep_name in cdeps_names:
+                    for cdep_name in impl['common_dep']:
                         cdep = upstreams[uloc]['commons'][cdep_name]
                         if 'required_flags' in cdep:
                             family['all_required_flags'].update(cdep['required_flags'])
diff --git a/scripts/copy_from_upstream/src/kem/family/CMakeLists.txt b/scripts/copy_from_upstream/src/kem/family/CMakeLists.txt
index 3393dc4e3..11e30320e 100644
--- a/scripts/copy_from_upstream/src/kem/family/CMakeLists.txt
+++ b/scripts/copy_from_upstream/src/kem/family/CMakeLists.txt
@@ -30,7 +30,7 @@ endif()
         {%- if impl['name'] == scheme['default_implementation'] %}
 
 if(OQS_ENABLE_KEM_{{ family }}_{{ scheme['scheme_c'] }}{%- if 'alias_scheme' in scheme %} OR OQS_ENABLE_KEM_{{ family }}_{{ scheme['alias_scheme'] }}{%- endif %})
-    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT kem_{{ family }}_{{ scheme['scheme'] }}.c {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'].replace(',', ' ').split() %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
+    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT kem_{{ family }}_{{ scheme['scheme'] }}.c {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'] %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
            {%- if impl['compile_opts'] %}
     target_compile_options({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PUBLIC {{ impl['compile_opts'] }})
            {%- endif -%}
@@ -51,14 +51,14 @@ if(OQS_ENABLE_KEM_{{ family }}_{{ scheme['scheme_c'] }}_{{ impl['name'] }}{%- if
         {%- else %}
 
 if(OQS_ENABLE_KEM_{{ family }}_{{ scheme['scheme_c'] }}_{{ impl['name'] }}{%- if 'alias_scheme' in scheme %} OR OQS_ENABLE_KEM_{{ family }}_{{ scheme['alias_scheme'] }}_{{ impl['name'] }}{%- endif %})
-    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'].replace(',', ' ').split() %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
+    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'] %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
         {%- endif %}
         {%- if impl['name'] != 'cuda' and impl['name'] != 'icicle_cuda' %}
     target_include_directories({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PRIVATE ${CMAKE_CURRENT_LIST_DIR}/{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }})
     target_include_directories({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PRIVATE ${PROJECT_SOURCE_DIR}/src/common/pqclean_shims)
         {%- if common_deps is defined %}
             {%- for cdep in common_deps %}
-                {%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'].replace(',', ' ').split() %}
+                {%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'] %}
     target_include_directories({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PRIVATE ${CMAKE_CURRENT_LIST_DIR}/{{ upstream_location }}_{{ cdep['name'] }})
                 {%- endif %}
             {%- endfor %}
diff --git a/scripts/copy_from_upstream/src/sig/family/CMakeLists.txt b/scripts/copy_from_upstream/src/sig/family/CMakeLists.txt
index 510aaaa27..d5db314e2 100644
--- a/scripts/copy_from_upstream/src/sig/family/CMakeLists.txt
+++ b/scripts/copy_from_upstream/src/sig/family/CMakeLists.txt
@@ -30,7 +30,7 @@ endif()
         {%- if impl['name'] == scheme['default_implementation'] %}
 
 if(OQS_ENABLE_SIG_{{ family }}_{{ scheme['scheme_c'] }}{%- if 'alias_scheme' in scheme %} OR OQS_ENABLE_SIG_{{ family }}_{{ scheme['alias_scheme'] }}{%- endif %})
-    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT sig_{{ family }}_{{ scheme['scheme'] }}.c {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'].replace(',', ' ').split() %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
+    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT sig_{{ family }}_{{ scheme['scheme'] }}.c {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'] %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
            {%- if impl['compile_opts'] %}
     target_compile_options({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PUBLIC {{ impl['compile_opts'] }})
            {%- endif -%}
@@ -38,13 +38,13 @@ if(OQS_ENABLE_SIG_{{ family }}_{{ scheme['scheme_c'] }}{%- if 'alias_scheme' in
         {%- else %}
 
 if(OQS_ENABLE_SIG_{{ family }}_{{ scheme['scheme_c'] }}_{{ impl['name'] }}{%- if 'alias_scheme' in scheme %} OR OQS_ENABLE_SIG_{{ family }}_{{ scheme['alias_scheme'] }}_{{ impl['name'] }}{%- endif %})
-    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'].replace(',', ' ').split() %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
+    add_library({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} OBJECT {% for source_file in impl['sources']|sort -%}{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }}/{{ source_file }}{%- if not loop.last %} {% endif -%}{%- endfor -%}{%- if common_deps is defined %}{%- for cdep in common_deps %}{%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'] %} {% for sf in cdep['sources_addl']|sort -%}{{ upstream_location }}_{{ cdep['name'] }}/{{ sf }}{%- if not loop.last %} {% endif -%}{%- endfor %}{%- endif %}{%- endfor %}{%- endif -%})
         {%- endif %}
     target_include_directories({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PRIVATE ${CMAKE_CURRENT_LIST_DIR}/{{ impl['upstream']['name'] }}_{{ scheme['pqclean_scheme'] }}_{{ impl['name'] }})
     target_include_directories({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PRIVATE ${PROJECT_SOURCE_DIR}/src/common/pqclean_shims)
         {%- if common_deps is defined %}
             {%- for cdep in common_deps %}
-                {%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'].replace(',', ' ').split() %}
+                {%- if cdep['include_only'] and 'common_dep' in impl and cdep['name'] in impl['common_dep'] %}
     target_include_directories({{ family }}_{{ scheme['scheme'] }}_{{ impl['name'] }} PRIVATE ${CMAKE_CURRENT_LIST_DIR}/{{ upstream_location }}_{{ cdep['name'] }})
                 {%- endif %}
             {%- endfor %}

It seems like there is more code change in this pull request than #2385.

[bhess]

It seems like there is more code change in this pull request than #2385.

This change addresses your remaining inline comment @xuganyu96 #2382 (comment), which wasn’t picked up in #2385. Would you be okay merging this PR to avoid leaving that item unaddressed?

(pushed an update rebasing it with main)

[xuganyu96]

@bhess The code change looks good to me. Please rebase on top of main and do a pipeline run, after which I can approve this pull request.