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

[install] MAKEFLAGS not propagated from toplevel Makefile to CMake generated makefiles #804

Closed
Jochen0x90h opened this issue May 14, 2019 · 6 comments · Fixed by #935
Closed

Comments

@Jochen0x90h
Copy link
Contributor

In the toplevel Makefile, the varialbe MAKEFLAGS gets assigned but is not used. Only CMAKEFLAGS is used but not assigned somewhere. Seems to be a minor thing but I'd like to add -DCMAKE_INSTALL_PREFIX="${HOME}/.local" to this variable

I also added an install target
install: build/Release
@$(MAKE) -C build/Release install

Now I can simply type make install

@xor-gate
Copy link
Member

Yeah the toplevel makefile is just for convenience so people don't have to type whole cmake & make commands. It sounds like a good addition to update the propagation of the variable from the toplevel makefile. You may proved a pull-request for this if you want.

@xor-gate xor-gate changed the title MAKEFLAGS not used in toplevel Makefile MAKEFLAGS not propagated from toplevel Makefile to CMake generated makefiles May 14, 2019
@Nightwalker-87 Nightwalker-87 added this to the General milestone Feb 19, 2020
@Nightwalker-87 Nightwalker-87 modified the milestones: General, v1.6.1 Feb 22, 2020
@Nightwalker-87 Nightwalker-87 modified the milestones: v1.6.1, Feedback required Mar 20, 2020
@Nightwalker-87 Nightwalker-87 modified the milestones: Feedback required, v1.6.1 Mar 28, 2020
@Nightwalker-87 Nightwalker-87 added this to To do in Release v1.6.1 via automation Mar 28, 2020
@Nightwalker-87 Nightwalker-87 changed the title MAKEFLAGS not propagated from toplevel Makefile to CMake generated makefiles [install] MAKEFLAGS not propagated from toplevel Makefile to CMake generated makefiles Mar 28, 2020
@Nightwalker-87 Nightwalker-87 moved this from To do to In progress in Release v1.6.1 Apr 5, 2020
@FreddieOliveira
Copy link

@Jochen0x90h and how exactly do you suggest to pass cmake variables, such as DCMAKE_INSTALL_PREFIX, via the top level Makefile?

As @xor-gate said, this top level Makefile is just for convenience, not for fine control over the build process. It automates the process for general cases, it wasn't intended for specific ones like yours. If you want granularity control over what cmake will do, you should run it yourself with the desired flags.

I do agree though that the top level Makefile could have the install and uninstall targets. But even this small change must be analyzed carefully. I.e., if I type make install for this top level Makefile, which build must it install, the Release build or the Debug one? Your workaround

install: build/Release
    @$(MAKE) -C build/Release install

would always install the Release build, but what if the person compiled the Debug one with make debug? The dir build/Release wouldn't even exist to start with and this target would fail.

@Nightwalker-87
Copy link
Member

I think you may not get an answer from Jochen0x90h as he no longer has a fork of this project.
Not sure if he is even notified then...

@Nightwalker-87 Nightwalker-87 removed their assignment Apr 9, 2020
@Nightwalker-87 Nightwalker-87 moved this from In progress to To do in Release v1.6.1 Apr 9, 2020
@Nightwalker-87
Copy link
Member

@xor-gate: To me this appears to be a valuable addition to the tools under the circumstance that there really is a way to make cmake accept (:laughing:) such commands from the toplevel makefile.

Do we have anybody here who is a kind of expert on this topic and can help out with a minor tweak?

@Nightwalker-87
Copy link
Member

@chenguokai: There is currently some refactoring on the way to improve the cmake compilation in a few parts. I'll upload this to a separate branch (build-settings). Maybe you can have a look at it. I assume it will not be fully functional from the start and that there will be a few leftovers to fix. If you fancy, you may have a look at it. Further it would make sense to address this ticket in that branch as well later on.

@Jochen0x90h
Copy link
Contributor Author

I added a pull request to the build-settings branch

@Nightwalker-87 Nightwalker-87 moved this from To do to Review in progress in Release v1.6.1 Apr 19, 2020
@Nightwalker-87 Nightwalker-87 linked a pull request Apr 19, 2020 that will close this issue
Release v1.6.1 automation moved this from Review in progress to Done Apr 20, 2020
@stlink-org stlink-org locked as resolved and limited conversation to collaborators Apr 27, 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 a pull request may close this issue.

4 participants