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

Use MAKE variable to pass correct info to child. #2396

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented May 16, 2022

Spawning a make subprocess should be done through use of the $(MAKE) variable so that the child process receives all the information it needs. closes #2395. I didn't check the whole list of make targets, but more could be affected.

Spawning a make subprocess should be done through use of the `$(MAKE)` variable so that the child process receives all the information it needs.
@Helveg
Copy link
Contributor Author

Helveg commented May 16, 2022

Point of discussion: This syntax doesn't seem to sit well with ninja, which complains:

ninja: error: build.ninja:126: bad $-escape (literal $ must be written as $$)

I have tried any combination of unqouted, quoted, bracket quoted, single $, double $, \\$, and VERBATIM you could think of, but can't find any syntax that both cmake and ninja will run:

  • If cmake runs it, ninja complains about escaping
  • If ninja runs it, various cmake configuration failures occur and the install target no longer exists.

Is it possible for the install-nodoc target to set the NEST_INSTALL_NODOC env var in another way, without having to subprocess a make process?

@terhorstd terhorstd added T: Bug Wrong statements in the code or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jun 27, 2022
@terhorstd terhorstd added this to To do in Build system and CI via automation Jun 27, 2022
@terhorstd
Copy link
Contributor

@jougs and I had a long discussion some time ago, that this should probably not be a cmake target at all. The concept of "nodoc" should be a flag that can be set. This would reduce the complexity of the issue a lot. We wrote this down in an issue, but never got around to working on this, also I seem to be unable to find the issue that I have in mind :-/ … does anyone reading this remember the discussion? @gtrensch maybe?

@gtrensch
Copy link
Contributor

gtrensch commented Jul 1, 2022

We discussed this in issue #1905.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Given the time passing on #1905 and the negligible amount of change this PR does, I would say this can go in and we tackle #1905 separately. This one doesn't solve the problem, but it does no harm and makes #2395 work for the time being.

@terhorstd terhorstd requested a review from gtrensch August 2, 2022 07:01
@Helveg
Copy link
Contributor Author

Helveg commented Aug 2, 2022

Well it breaks make install-nodoc for ninja users 😟

Copy link
Contributor

@gtrensch gtrensch left a comment

Choose a reason for hiding this comment

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

Approving.

@gtrensch gtrensch merged commit 5350de0 into nest:master Aug 16, 2022
Build system and CI automation moved this from To do to Done Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Development

Successfully merging this pull request may close these issues.

Can't use -j on install-nodoc target
3 participants