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

st-flash and other utilities search for chip files in the wrong directory #1180

Closed
slyshykO opened this issue Aug 15, 2021 · 13 comments
Closed

Comments

@slyshykO
Copy link
Collaborator

  • OS: Linux on ARM
  • version 1.7.0-105-gdb8f789 (lastest develop)
  • st-info, st-flash, st-trace, st-util

make install copied chip's files to /usr/local/etc/stlink/chips but st-* utils search them in /etc/stlink/chips

@Nightwalker-87
Copy link
Member

Another classical example for unfinished attempts... :-/
Regression from #1129.

@slyshykO
Copy link
Collaborator Author

Another classical example for unfinished attempts... :-/

Don't be discouraged. This is a typical process in software development :)

@Nightwalker-87
Copy link
Member

Yes, but it should appear on private/personal branches with intermediate working (sub)sets to be proposed for a merge each and not within endlessly reworked PRs that then appear to stall. That's the point and this is actually causing such problems. PRs are not spaces for development, but for final review and last additional changes.

I just find this regression deriving from such a background - that's why.

@Nightwalker-87
Copy link
Member

The hardcoded path /etc/stlink/chips is definitely wrong - we shall never use hardcoded paths without the ability to define a variable cmake prefix.

Thus changing install(FILES ${CHIP_FILES} DESTINATION ${CMAKE_ETC_CHIPS_DIR})

into install(FILES ${CHIP_FILES} DESTINATION ${CMAKE_INSTALL_PREFIX}/${CMAKE_ETC_CHIPS_DIR})

within /CMakeLists.txt should actually fix this. Please let me know about the result.
I'd provide a fix then with some other minor clean-up as well.

@Nightwalker-87 Nightwalker-87 self-assigned this Aug 16, 2021
@Nightwalker-87
Copy link
Member

Another leftover is the old chip configuration in /src/stlink-lib/chipid.c - it should be removed after a comparison of the respective MCU configuration and no longer remain as a duplicate. Otherwise it will simply stay there forever... I still don't understand why this has not taken place during the move to the file-based configuration - there is no rational explanation for this.

@gszy
Copy link
Collaborator

gszy commented Aug 21, 2021

Here, FWIW, is a patch that replaces all configs/chips/* files with ones freshly generated from src/stlink-lib/chipid.c: 0001-Update-chip-config-files-from-the-library-struct.patch.gz.

@Nightwalker-87
Copy link
Member

@gszy Good attempt! 👍
I've just had a look at it, but propose that you submit it via a separate PR, as it changes quite a few files.
I wouldn't want to mix or submit it with changes I am currently implementing.

@Nightwalker-87
Copy link
Member

@slyshykO Can you confirm that #1180 (comment) fixes this?

@slyshykO
Copy link
Collaborator Author

@slyshykO Can you confirm that #1180 (comment) fixes this?

No, this is not enough.
There is also need to move this

set(CMAKE_STLINK_ETC_DIR  etc)
set(CMAKE_ETC_CHIPS_DIR  ${CMAKE_STLINK_ETC_DIR}/stlink/chips)
set(CMAKE_ETC_CHIPS_DIR_ABS  ${CMAKE_INSTALL_PREFIX}/${CMAKE_ETC_CHIPS_DIR})
add_definitions( -DETC_STLINK_DIR="${CMAKE_ETC_CHIPS_DIR_ABS}" )

down, right after

include(GNUInstallDirs) # Define GNU standard installation directories

Also, I think that we should use share dir name, not etc here

set(CMAKE_STLINK_ETC_DIR  etc)

@Nightwalker-87
Copy link
Member

Yes, we should definitely use share.

@gszy
Copy link
Collaborator

gszy commented Aug 22, 2021

IMHO, ideally we would have the default chip files installed to $PREFIX/share, but read them from /etc (and/or perhaps $XDG_DATA_HOME) as well. Alternatively, at least provide a command line option to override the chips config directory at runtime. This is probably a separate issue, but something to consider when choosing names like CMAKE_ETC_CHIPS_DIR (note the “ETC”).

@Nightwalker-87
Copy link
Member

Well, I was just going to change that naming anyway due to the mentioned reason...

@Nightwalker-87
Copy link
Member

My suggestion is:

## MCU configuration files
set(CMAKE_CHIPS_SUBDIR stlink/chips)
set(CMAKE_CHIPS_DIR  ${CMAKE_INSTALL_PREFIX}/${CMAKE_CHIPS_SUBDIR})
add_definitions( -DETC_STLINK_DIR="${CMAKE_CHIPS_DIR}" )

just after include(GNUInstallDirs) # Define GNU standard installation directories
and with updating the command in the install section further downwards as well:

# Install MCU configuration files
file(GLOB CHIP_FILES ${CMAKE_SOURCE_DIR}/config/chips/*.chip)
install(FILES ${CHIP_FILES} DESTINATION ${CMAKE_CHIPS_DIR})

@gszy Additionally we should talk about the additional read from /etc as well, but for that we should clarify first under which circumstances this directory may come into use (because unless this is an external requirement from a distro, we should not have it set from my point of view).

Nightwalker-87 added a commit that referenced this issue Aug 29, 2021
- Corrected paths for chip-id files (Closes #1180)
- Fixed 32-bit build (Closes #985) (Closes #1175)
- Patch for GitHub Actions Workflow (Ubuntu)
Nightwalker-87 added a commit that referenced this issue Aug 29, 2021
- Corrected paths for chip-id files (Closes #1180)
- Fixed 32-bit build (Closes #985) (Closes #1175)
- Patch for GitHub Actions Workflow (Ubuntu)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.