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.

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

version: use generated version_def.h #469

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Conversation

kernelchuk
Copy link
Contributor

Using command line to define the OPENAMP_VERSION string macro breaks Cygwin build on some releases of Microsoft Windows and CMake. Remove all OPENAMP_VERSION macros from cmake compiler flags and use the ones from the generated version_def.h file.

Using command line to define the OPENAMP_VERSION string macro breaks
Cygwin build on some releases of Microsoft Windows and CMake. Remove all
OPENAMP_VERSION macros from cmake compiler flags and use the ones from
the generated version_def.h file.

Signed-off-by: Sergei Korneichuk <sergei.korneichuk@amd.com>
Copy link
Collaborator

@tnmysh tnmysh left a 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.

@arnopo arnopo requested review from arnopo and edmooring March 3, 2023 12:32
@arnopo
Copy link
Collaborator

arnopo commented Mar 3, 2023

Please Could you please provide more details on the issue ? I would like to understand the problem you are facing.

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

This change looks reasonable, but I'd like to see more detail about that goes wrong with Cygwin in the commit message.

@kernelchuk
Copy link
Contributor Author

kernelchuk commented Mar 8, 2023

The error

src/open-amp/lib/version.c: In function 'openamp_version':
        <command-line>: error: too many decimal points in number
C:src/open-amp/lib/version.c:24:16: note: in expansion of macro 'OPENAMP_VERSION'
24 |         return OPENAMP_VERSION;
   |                ^~~~~~~~~~~~~~~

happens during open-amp build in Vitis on MS-Windows under Cygwin. The top-level make is started via

make -C psu_cortexr5_0/libsrc/openamp_v1_8/src -s libs "SHELL=CMD"
  "COMPILER=armr5-none-eabi-gcc" "ASSEMBLER=armr5-none-eabi-as"  "ARCHIVER=armr5-none-eabi-ar"
  "COMPILER_FLAGS= -O2 -c -mcpu=cortex-r5" ...

where each compiler option is enclosed in double quotes. This collides with the quotes from

add_definitions( -DOPENAMP_VERSION="${PROJECT_VERSION}" )
One option is to try to get the quotes to work for both Linux and Windows build. The other is to follow the libmetal example of lib/config.h. The open-amp repo does not have config.h, but it generates version_def.h from VERSION. This way we avoid the build system shell dependencies and rely only on the CMake and the preprocessor.

@arnopo
Copy link
Collaborator

arnopo commented Mar 14, 2023

The error

src/open-amp/lib/version.c: In function 'openamp_version':
        <command-line>: error: too many decimal points in number
C:src/open-amp/lib/version.c:24:16: note: in expansion of macro 'OPENAMP_VERSION'
24 |         return OPENAMP_VERSION;
   |                ^~~~~~~~~~~~~~~

happens during open-amp build in Vitis on MS-Windows under Cygwin. The top-level make is started via

make -C psu_cortexr5_0/libsrc/openamp_v1_8/src -s libs "SHELL=CMD"
  "COMPILER=armr5-none-eabi-gcc" "ASSEMBLER=armr5-none-eabi-as"  "ARCHIVER=armr5-none-eabi-ar"
  "COMPILER_FLAGS= -O2 -c -mcpu=cortex-r5" ...

where each compiler option is enclosed in double quotes. This collides with the quotes from

add_definitions( -DOPENAMP_VERSION="${PROJECT_VERSION}" )

One option is to try to get the quotes to work for both Linux and Windows build. The other is to follow the libmetal example of lib/config.h. The open-amp repo does not have config.h, but it generates version_def.h from VERSION. This way we avoid the build system shell dependencies and rely only on the CMake and the preprocessor.

Thanks for the explanation, it's much clearer now.
Regarding some posts, seems that another way to fix this is to use
add_definitions( -DOPENAMP_VERSION="\\"${PROJECT_VERSION}\\"" )

That said, I can not find any reason to keep definitions in cmake, so your proposal seems to me ok

arnopo
arnopo approved these changes Mar 14, 2023
@arnopo arnopo requested a review from edmooring March 23, 2023 08:35
@arnopo arnopo added this to the Release V2023.04 milestone Mar 23, 2023
@arnopo
Copy link
Collaborator

arnopo commented Apr 20, 2023

@edmooring can you give a feedback on this one as it is a candidate for the release?
Thanks!

Copy link
Contributor

@edmooring edmooring left a 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.

@arnopo arnopo merged commit 14f3f84 into OpenAMP:main Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants