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

Fixes default paths with empty ENVs, and Checks paths for existence. #244

Merged
merged 1 commit into from
Aug 1, 2021

Conversation

BenArtes
Copy link

@BenArtes BenArtes commented Jul 22, 2021

Closes #240, by checking ENV path exists before setting it

@BenArtes BenArtes changed the title Fixes Defaults added in PR#226 not working for empty ENV Variables Fixes Defaults with empty ENVs, and Checks paths for existence. Jul 22, 2021
@BenArtes BenArtes changed the title Fixes Defaults with empty ENVs, and Checks paths for existence. Fixes default paths with empty ENVs, and Checks paths for existence. Jul 22, 2021
@BenArtes
Copy link
Author

I Updated the PR based on discussion in #240 to use defined ENV check instead of CMake set FORCE optional.

On defined ENV a message will be printed to indicate ENV path is being used. If not defined default gets used. After, existence is checked and fatal error thrown if path doesn't exist.

Printing ENV message allows single check at the end rather than multiple existence checks.

message(STATUS "Neither STM32_CUBE_${FAMILY}_PATH nor STM32_CMSIS_${FAMILY}_PATH specified using default STM32_CUBE_${FAMILY}_PATH: ${STM32_CUBE_${FAMILY}_PATH}")
endif()

if (NOT EXISTS ${STM32_CUBE_${FAMILY}_PATH})
Copy link
Collaborator

Choose a reason for hiding this comment

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

you must check that none of path exists. One is needed but it must not be cube in all cases

Copy link
Author

Choose a reason for hiding this comment

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

Added check for STM32_CMSIS_${FAMILY}_PATH and updated FATAL_ERROR message.

endif()

if(NOT FREERTOS_PATH)
set(FREERTOS_PATH /opt/FreeRTOS CACHE PATH "Path to FreeRTOS")
set(FREERTOS_PATH /opt/FreeRTOS CACHE PATH "Path to FreeRTOS" FORCE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
set(FREERTOS_PATH /opt/FreeRTOS CACHE PATH "Path to FreeRTOS" FORCE)
set(FREERTOS_PATH /opt/FreeRTOS CACHE PATH "Path to FreeRTOS")

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

endif()

if((NOT STM32_HAL_${FAMILY}_PATH) AND (NOT STM32_CUBE_${FAMILY}_PATH))
set(STM32_CUBE_${FAMILY}_PATH /opt/STM32Cube${FAMILY} CACHE PATH "Path to STM32Cube${FAMILY}")
message(STATUS "Neither STM32_CUBE_${FAMILY}_PATH nor STM32_HAL_${FAMILY}_PATH specified using default STM32_CUBE_${FAMILY}_PATH: ${STM32_CUBE_${FAMILY}_PATH}")
endif()

if (NOT EXISTS ${STM32_CUBE_${FAMILY}_PATH})
Copy link
Collaborator

Choose a reason for hiding this comment

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

both paths must be missing to have FATAL_ERROR

Copy link
Author

Choose a reason for hiding this comment

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

Added check for STM32_HAL_${FAMILY}_PATH and updated FATAL_ERROR message.

@atsju atsju requested review from Hish15 and atsju July 22, 2021 18:12
@atsju
Copy link
Collaborator

atsju commented Jul 22, 2021

I just launched the actions. The tests must all pass to merge.
The easy solution would be to remove fatal errors but it's better if we can understand why exactly it doesn't pass.

Is it possible that the fetch sets the path only after the check ?

@BenArtes
Copy link
Author

It seems like test/fetch runs for each device family. For each device family it fetches the necessary CMSIS / HAL then runs FindCMSIS.cmake etc.

Because the COMPONENTS aren't specified in test/fetch's find_package FindCMSIS.cmake iterates over all possible components:

FindCMSIS.cmake

if(NOT CMSIS_FIND_COMPONENTS)
    set(CMSIS_FIND_COMPONENTS ${STM32_SUPPORTED_FAMILIES_LONG_NAME})
endif()
...
foreach(COMP ${CMSIS_FIND_COMPONENTS})

This can be seen in previous test output:

[100%] Built target stm32-hal-f0-populate
-- Neither STM32_CUBE_F1_PATH nor STM32_CMSIS_F1_PATH specified using default  STM32_CUBE_F1_PATH: 
-- Neither STM32_CUBE_F2_PATH nor STM32_CMSIS_F2_PATH specified using default  STM32_CUBE_F2_PATH: 
-- Neither STM32_CUBE_F3_PATH nor STM32_CMSIS_F3_PATH specified using default  STM32_CUBE_F3_PATH: 
-- Neither STM32_CUBE_F4_PATH nor STM32_CMSIS_F4_PATH specified using default  STM32_CUBE_F4_PATH: 
-- Neither STM32_CUBE_F7_PATH nor STM32_CMSIS_F7_PATH specified using default  STM32_CUBE_F7_PATH: 
-- Neither STM32_CUBE_G0_PATH nor STM32_CMSIS_G0_PATH specified using default  STM32_CUBE_G0_PATH: 
-- Neither STM32_CUBE_G4_PATH nor STM32_CMSIS_G4_PATH specified using default  STM32_CUBE_G4_PATH: 
-- Neither STM32_CUBE_H7_PATH nor STM32_CMSIS_H7_PATH specified using default  STM32_CUBE_H7_PATH: 
-- Neither STM32_CUBE_H7_PATH nor STM32_CMSIS_H7_PATH specified using default  STM32_CUBE_H7_PATH: 
-- Neither STM32_CUBE_L0_PATH nor STM32_CMSIS_L0_PATH specified using default  STM32_CUBE_L0_PATH: 
-- Neither STM32_CUBE_L1_PATH nor STM32_CMSIS_L1_PATH specified using default  STM32_CUBE_L1_PATH: 
-- Neither STM32_CUBE_L4_PATH nor STM32_CMSIS_L4_PATH specified using default  STM32_CUBE_L4_PATH: 
-- Neither STM32_CUBE_L5_PATH nor STM32_CMSIS_L5_PATH specified using default  STM32_CUBE_L5_PATH: 
-- Found CMSIS: /home/runner/work/stm32-cmake/tests/fetch/build/_deps/stm32-cmsis-src/Include;/home/runner/work/stm32-cmake/tests/fetch/build/_deps/stm32-cmsis-f0-src/Include (found version "5.6.0") found components: STM32F0 missing components: STM32F1 STM32F2 STM32F3 STM32F4 STM32F7 STM32G0 STM32G4 STM32H7_M4 STM32H7_M7 STM32L0 STM32L1 STM32L4 STM32L5
-- Search for HAL families: STM32F0;STM32F1;STM32F2;STM32F3;STM32F4;STM32F7;STM32G0;STM32G4;STM32H7_M4;STM32H7_M7;STM32L0;STM32L1;STM32L4;STM32L5
-- Search for HAL drivers: adc;can;cec;comp;cortex;crc;dac;dma;exti;flash;gpio;i2c;i2s;irda;iwdg;pcd;pwr;rcc;rtc;smartcard;smbus;spi;tim;tsc;uart;usart;wwdg;eth;hcd;mmc;nand;nor;pccard;sd;sram;cryp;dcmi;hash;rng;hrtim;opamp;sdadc;dfsdm;dma2d;dsi;flash_ramfunc;fmpi2c;lptim;ltdc;qspi;sai;sdram;spdifrx;jpeg;mdios;cordic;fdcan;fmac;dts;gfxmmu;hsem;mdma;ospi;otfdec;pssi;ramecc;swpmi;firewall;lcd;pka;gtzc;icache
-- Search for HAL LL drivers: adc;comp;crc;crs;dac;dma;exti;gpio;i2c;pwr;rcc;rtc;spi;tim;usart;usb;utils;fsmc;sdmmc;rng;fmc;hrtim;opamp;dma2d;lptim;lpuart;ucpd;cordic;fmac;bdma;delayblock;mdma;swpmi;pka
-- Could not read the HAL version from package.xml for STM32F0
-- Neither STM32_CUBE_F1_PATH nor STM32_HAL_F1_PATH specified using default STM32_CUBE_F1_PATH: 
-- Could not read the HAL version from package.xml for STM32F1
-- Neither STM32_CUBE_F2_PATH nor STM32_HAL_F2_PATH specified using default STM32_CUBE_F2_PATH: 
-- Could not read the HAL version from package.xml for STM32F2
-- Neither STM32_CUBE_F3_PATH nor STM32_HAL_F3_PATH specified using default STM32_CUBE_F3_PATH: 
-- Could not read the HAL version from package.xml for STM32F3
-- Neither STM32_CUBE_F4_PATH nor STM32_HAL_F4_PATH specified using default STM32_CUBE_F4_PATH: 
-- Could not read the HAL version from package.xml for STM32F4
-- Neither STM32_CUBE_F7_PATH nor STM32_HAL_F7_PATH specified using default STM32_CUBE_F7_PATH: 
-- Could not read the HAL version from package.xml for STM32F7
-- Neither STM32_CUBE_G0_PATH nor STM32_HAL_G0_PATH specified using default STM32_CUBE_G0_PATH: 
-- Could not read the HAL version from package.xml for STM32G0
-- Neither STM32_CUBE_G4_PATH nor STM32_HAL_G4_PATH specified using default STM32_CUBE_G4_PATH: 
-- Could not read the HAL version from package.xml for STM32G4
-- Neither STM32_CUBE_H7_PATH nor STM32_HAL_H7_PATH specified using default STM32_CUBE_H7_PATH: 
-- Could not read the HAL version from package.xml for STM32H7_M4
-- Neither STM32_CUBE_H7_PATH nor STM32_HAL_H7_PATH specified using default STM32_CUBE_H7_PATH: 
-- Could not read the HAL version from package.xml for STM32H7_M7
-- Neither STM32_CUBE_L0_PATH nor STM32_HAL_L0_PATH specified using default STM32_CUBE_L0_PATH: 
-- Could not read the HAL version from package.xml for STM32L0
-- Neither STM32_CUBE_L1_PATH nor STM32_HAL_L1_PATH specified using default STM32_CUBE_L1_PATH: 
-- Could not read the HAL version from package.xml for STM32L1
-- Neither STM32_CUBE_L4_PATH nor STM32_HAL_L4_PATH specified using default STM32_CUBE_L4_PATH: 
-- Could not read the HAL version from package.xml for STM32L4
-- Neither STM32_CUBE_L5_PATH nor STM32_HAL_L5_PATH specified using default STM32_CUBE_L5_PATH: 
-- Could not read the HAL version from package.xml for STM32L5
-- Found HAL: /home/runner/work/stm32-cmake/tests/fetch/build/_deps/stm32-hal-f0-src/Inc   
-- Configuring done
-- Generating done
-- Build files have been written to: /home/runner/work/stm32-cmake/tests/fetch/build

Seems like the test needs to be modifed to populate the correct COMPONENTS based on TEST_FAMILIES. I'm not familiar enough with CMake Syntax to figure out how to do that idiomatically; my first hack would be to create a list, iterate over TEST_FAMILIES, and append to the list.

@BenArtes
Copy link
Author

I found out how to idiomatically prepend lists and have pushed an update to test.

Two notes:

  1. I wasn't entirely sure how to handle COMPONENTS in test/hal with the FETCH_ST_SOURCES variable, for now I've assumed that based on other usage of TEST_FAMILIES, we still want to add COMPONENTS.
  2. Depending on user usage of COMPONENTS, this FATAL_ERROR change could make the existence check a breaking change. Users who haven't added COMPONENTS may have working builds right now because all of the other builds silently fail; adding the FATAL_ERROR will break the build for those users.

@atsju
Copy link
Collaborator

atsju commented Jul 23, 2021

Thank you for update.
Indeed this breaks a feature according to Readme :

You can specify STM32 family or even specific device (STM32F407VG) in COMPONENTS or omit COMPONENTS totally - in that case stm32-cmake will find ALL sources for ALL families and ALL chips (you'll need ALL STM32Cube packages somewhere).

@Hish15 what do you think of this ?
FATAL_ERROR + readme update VS no FATAL_ERROR ?

@atsju
Copy link
Collaborator

atsju commented Jul 23, 2021

@BenArtes
according to the documentation
FATAL_ERROR should not always be applied. The modification you did to the tests was needed as the components were marked REQUIRED or we could just remove the REQUIRED in tests but there should still be a slight modification because without REQUIRED there should not be FATAL_ERROR

Briefly, the module should only locate versions of the package compatible with the requested version, as described by the Foo_FIND_VERSION family of variables. If Foo_FIND_QUIETLY is set to true, it should avoid printing messages, including anything complaining about the package not being found. If Foo_FIND_REQUIRED is set to true, the module should issue a FATAL_ERROR if the package cannot be found. If neither are set to true, it should print a non-fatal message if it cannot find the package.

Thank you for your time and sorry if this is a bit long. We try to avoid breaking changes and partial fixes when we see something that needs improvements.

@BenArtes
Copy link
Author

I'm happy to make whatever changes are necessary.

I added the existence checks to this PR because they seemed relevant and like a small change; it now seems like they are a larger change that needs more discussion. Should I pull those changes to a new PR for that further discussion, leaving just the default value fix for this PR?

I think I know the solution for this issue, but we'll likely need to discuss test changes and document changes etc.

Side Note: I wish to cleanup this PR Branch to remove erroneous commits etc, I believe github handles force pushes correctly and will update the PR, should I go ahead and do that?

@atsju
Copy link
Collaborator

atsju commented Jul 23, 2021

Glad to read you are motivated to help.
Please force push the fix here. We will be able to merge faster.
To anyone reading this : we usually not like force push after reviews have been done but here it is small and will fix your wrong commit so it's recommended in this case.

Then do a different PR for the FATAL_ERROR thing. It will need to be fatal error only without REQUIRED like mentioned here : https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#

@BenArtes BenArtes force-pushed the fix_default_by_set_force branch from e69373d to 278c875 Compare July 23, 2021 13:30
@atsju
Copy link
Collaborator

atsju commented Jul 26, 2021

@Hish15 Go

@Hish15 Hish15 merged commit 238c0fe into ObKo:master Aug 1, 2021
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.

Default STM32_CUBE_family_PATH is not being set
3 participants