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

[refactoring] Cleanup of cmake build process #947

Merged
merged 7 commits into from Apr 26, 2020
Merged

[refactoring] Cleanup of cmake build process #947

merged 7 commits into from Apr 26, 2020

Conversation

Nightwalker-87
Copy link
Member

Refactoring for cmake build settings:

  • Moved additional header files to /src
  • Simplified header-paths
  • Added missing license title for getopt
  • Added comments and sections to CMakeLists
  • Improved file structure (partially)
  • Co-build st-util with st-info & st-flash
  • GUI: Renamed .ui file to match executable
  • Added install targets for executables
  • Added missing "uninstall" cmd to Makefile
  • Distinguish test binaries from binaries
  • Build all binaries to /bin subfolder
  • Added comments to explain settings
  • Moved header files for logging and mmap
  • Removed requirement for 7zip on Windows
  • Added note on GNUInstallDir presets ([install] CMakeLists.txt hardcodes /etc/... paths #557)

The exctraction of the libusb library archive on windows no longer 
requires an external unarchiver.
- Removed requirement for 7zip on Windows
- Added note on GNUInstallDir presets (#557)
- Distinguish test binaries from binaries
- Build all binaries to /bin subfolder
- Added comments to explain settings
- Moved header files for logging and mmap
- Moved additional header files to /src
- Simplified header-paths
- Added missing license title for getopt
- Added comments and sections to CMakeLists
- Improved file structure (partially)
- Co-build st-util with st-info & st-flash
- GUI: Renamed .ui file to match executable
- Added install targets for executables
- Added missing "uninstall" cmd to Makefile
@Nightwalker-87
Copy link
Member Author

This is about 4/5 of the planned work towards a clean and more structured build routine.
We want some testing for this on linux (fedora), macOS and windows...
Also constructive feedback is highly welcome.

@Nightwalker-87 Nightwalker-87 added this to Review in progress in Release v1.6.1 Apr 25, 2020
@Nightwalker-87 Nightwalker-87 linked an issue Apr 25, 2020 that may be closed by this pull request
@chenguokai
Copy link
Collaborator

Both stlink-gui and stlink-gui.ui is installed to PREFIX/bin as expected, while if I launch stlink-gui from other directories, either by adding PREFIX/bin to PATH or using absolute path, stlink-gui throws an error "Failed to load UI file: stlink-gui.ui".

I suppose it is associated with this change, though may not actually caused by the cleanup.

By checking the strings in the binary file, I find stlink-gui seems to have ignored my prefix setting -DCMAKE_INSTALL_PREFIX=/Users/cgk/Downloads/stlink-pkg/dst/. The expected path of stlink-gui.ui is still /usr/local/bin/stlink-gui.ui rather than where all binaries have been installed to (/Users/cgk/Downloads/stlink-pkg/dst/)

@Nightwalker-87
Copy link
Member Author

I'll have a look at this. The problem you describe where st-link-gui could not find the ui file is indeed an old problem. I have seen that before. Against this background I considered to install to /usr/local/bin (/usr/local is the default GNUInstallDirs-Prefix), as this path should be listed in the system variable $PATH per default. I actually thought this would solve that old problem... :-/
We need some investigation here to achieve better usability.

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Apr 26, 2020

@chenguokai: Tested it again and I can confirm I am able to launch stlink-gui via command line from every directory. Precondition is to have /usr/local/bin in $PATH what should be the standard case.
Have you built with sudo make install? (By now you can run sudo make uninstall to cleanly remove all installed files again if you wish to do so.)

@chenguokai
Copy link
Collaborator

Precondition is to have /usr/local/bin in $PATH what should be the standard case.

I am afraid that this violates the MacPorts's (Maybe other package managers' as well) rule however. MacPorts expects every single file provided by a software package only resident in its ${PREFIX} which is by default /opt/local. If stlink-gui.ui is installed to /usr/local/bin, MacPorts would refuse this install destination by default.

If there should be one hard-coded path for stlink-gui.ui, I recommend using a relative path like WHERE_STLINK_GUI_IS/stlink-gui.ui which will prevent us from this error as long as the binary and the ui file are installed to the same directory.

@Nightwalker-87
Copy link
Member Author

I think the problem is something else, more general here: GNUInstallDirs should not be applied for macOS environments, as this sets the prefix to /usr/local. Reading your explanation implies to me that the prefix needs to change to /opt/local/usr/local on macOS. Then the file tree is preserved and remains identical to linux systems.

@chenguokai
Copy link
Collaborator

Reading your explanation implies to me that the prefix needs to change to /opt/local/usr/local

Please note that ${PREFIX} is only default to MacPorts. Users may adjust this ENV and HomeBrew has its own path convention. That’s why I proposed a relative path solution.

@Nightwalker-87
Copy link
Member Author

@chenguokai: Have a look at #557 also...

@Nightwalker-87
Copy link
Member Author

@Vascom: Can you test this on Fedora?

@Vascom
Copy link
Collaborator

Vascom commented Apr 26, 2020

I need a commit number to test.

@Nightwalker-87
Copy link
Member Author

Just use the latest commit of the pkg branch.

@chenguokai
Copy link
Collaborator

@Nightwalker-87 I checked the GNU standard, where they said

GNU packages should not try to guess which value should be appropriate for these variables on the system they are being installed onto: use the default settings specified here so that all GNU packages behave identically, allowing the installer to achieve any desired layout.

That is exactly what I expected. Using GNUInstallDirs is fine in my view. What I found improper is, the hardcoded /usr/local/bin/stlink-gui.ui even if a custom prefix is provided. I believe my intention is the same with #557, that is reducing hardcoded paths if possible.

@Nightwalker-87
Copy link
Member Author

@chenguokai: Ah now I see what you mean! 💡 Yes, of course we need a placeholder there... This is clearly by mistake. 😞

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Apr 26, 2020

What I found improper is, the hardcoded /usr/local/bin/stlink-gui.ui even if a custom prefix is provided.

Surprisingly it shouldn't be hardcoded, as I just found out: See /src/stlink-gui/CMakeLists.txt (Line 30). I don't know where this comes from. I thought it was accidentally hardcoded there... Lines 35/36 are implemented equally to the one for the tools in the main /CMakeLists.txt 😕
Still I believe there is an error somewhere...

@Vascom
Copy link
Collaborator

Vascom commented Apr 26, 2020

/usr/share/stlink/ removed?
Build successful but this folder not found.

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Apr 26, 2020

@Vascom: Yes, it is not there any more as it is no longer used. Does everything else work as expected?

@chenguokai
Copy link
Collaborator

@Nightwalker-87 I made one mistake in my verification. This worked as expected. No more issues.😅

Release v1.6.1 automation moved this from Review in progress to Reviewer approved Apr 26, 2020
@Vascom
Copy link
Collaborator

Vascom commented Apr 26, 2020

stlink-gui.ui file in /usr/bin/
It is normal?

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Apr 26, 2020

Yes, it needs to be in the same directory as the binary stlink-gui otherwise the GUI can not be called regardless of the current working directory. See also the related issue to this PR.

@Nightwalker-87 Nightwalker-87 linked an issue Apr 26, 2020 that may be closed by this pull request
@Vascom
Copy link
Collaborator

Vascom commented Apr 26, 2020

Now build OK.

@Nightwalker-87 Nightwalker-87 merged commit 5899409 into develop Apr 26, 2020
Release v1.6.1 automation moved this from Reviewer approved to Done Apr 26, 2020
@Nightwalker-87 Nightwalker-87 deleted the pkg branch April 26, 2020 14:45
@stlink-org stlink-org locked as resolved and limited conversation to collaborators Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[refactoring] Cleanup of cmake build process stlink.pc not installed
4 participants