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.

Sign up for GitHub

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

Jump to bottom

build-sys: fix invalid args detected by meson 0.42 #6561

Merged
merged 1 commit into from
Aug 9, 2017
Merged

build-sys: fix invalid args detected by meson 0.42 #6561

merged 1 commit into from
Aug 9, 2017

Conversation

userwithuid
Copy link
Contributor

not sure about the man/html custom_target.

the 3 removed args are redundant afaiu the meson docs.

keszybz
keszybz reviewed Aug 8, 2017
View reviewed changes
meson.build Outdated
@@ -2346,11 +2346,9 @@ if git.found()

run_target(
'tags',
input : all_files,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this just be changed to depends?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depends doesn't work with files, only with buildtarget.

man/meson.build Outdated
@@ -199,6 +199,5 @@ if git.found()
command : ['sh', '-c',
'cd @0@ && '.format(meson.build_root()) +
'python3 @0@/tools/make-man-rules.py `git ls-files ":/man/*.xml"` >t && '.format(meson.source_root()) +
'mv t @0@/rules/meson.build'.format(meson.current_source_dir())],
depend_files : custom_entities_ent)
Copy link
Member

Choose a reason for hiding this comment

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

You drop the dependency on custom_entities_ent, which is quite important… Do you think it's not possible to keep it?

@userwithuid
Copy link
Contributor Author

tbh, i didn't really try to figure what those things were supposed to do if they worked. :-) They are ignored silently in 0.41 (prob older versions too?), 0.42 just adds the warning. You can check builddir/ninja.build to see that.

We can make all of them custom_target to get an actual dep in the ninja file. Do you prefer that?

I noticed this would change ninja update-man-rules to ninja man/update-man-rules but seems to work otherwise.

@keszybz
Copy link
Member

keszybz commented Aug 9, 2017

I can't swear that all of this worked properly, but I'm sure that there have been some regression in meson. For example man target worked when I added it, and with the update to 0.40 or thereabouts it stopped.

I think custom_target is preferable. If you could update the patch that'd be great.

@userwithuid
build-sys: fix invalid args detected by meson 0.42
3b84684 
some run_target() calls were using params from custom_target()

example message:
WARNING: Passed invalid keyword argument "input". This will become a hard error in the future.
userwithuid
userwithuid commented Aug 9, 2017
View reviewed changes
meson.build
'tags',
input : all_files,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: Removing the input is intentional. custom_target (unlike run_target) automatically adds files appearing in command as implicit deps in ninja.build.

@keszybz
Copy link
Member

keszybz commented Aug 9, 2017

Fake output files, hmmm. That's not very pleasant, but as long as it works... ;]

@keszybz
Copy link
Member

keszybz commented Aug 9, 2017

'ninja -C build man/htmlandman/update-man-rulesworks nicely.man/man` works too. It would be nice to have top-level targets again, but it doesn't matter much.

@keszybz keszybz merged commit e85a690 into systemd:master Aug 9, 2017
@userwithuid userwithuid deleted the meson042-invalid-args branch August 9, 2017 16:40
@userwithuid
Copy link
Contributor Author

Yeah, it's not pretty to use output file names to create phony targets. Then again as long as meson has the bug you reported, run_target has some sort of implicit fake output file as well, not really much of a difference.

Wrt to top-level targets: If you are really into that, just s/man/mans/ (or manpages or whatever) to avoid the bug and back to run_target for that and html. For the last one, wrap a run_target around, e.g.

run_target('update-man-rules', depends: custom_gen_man_rules, command: ['true'])

This would also make it possible to stop misusing output and put something more file-y in there e.g. manrules.tmp, could even use it inside the command as @OUTPUT@ to give it purpose. On the flipside, this kinda misuses run_target as an alias, haha. Seems like a matter of preference, do you wanna do it this way?

@keszybz
Copy link
Member

keszybz commented Aug 9, 2017

Nah, I think things are fine as they are. Meson is changing quickly, so I'd just wait and revisit in a few months.

andir pushed a commit to andir/systemd that referenced this pull request Sep 7, 2017
@userwithuid @andir
build-sys: fix invalid args detected by meson 0.42 (systemd#6561)
fd5b684 
some run_target() calls were using params from custom_target()

example message:
WARNING: Passed invalid keyword argument "input". This will become a hard error in the future.

New way to call targets:
ninja man/man
ninja man/html
ninja man/update-man-rules
andir pushed a commit to andir/systemd that referenced this pull request Sep 22, 2017
@userwithuid @andir
build-sys: fix invalid args detected by meson 0.42 (systemd#6561)
0e87ca6 
some run_target() calls were using params from custom_target()

example message:
WARNING: Passed invalid keyword argument "input". This will become a hard error in the future.

New way to call targets:
ninja man/man
ninja man/html
ninja man/update-man-rules
@evverx evverx mentioned this pull request May 18, 2018
Open
evverx
evverx reviewed May 18, 2018
View reviewed changes
meson.build
command : ['env', 'etags', '-o', '@0@/TAGS'.format(meson.source_root())] + all_files)
run_target(
custom_target(
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about tags, but ctags seems to be broken due to mesonbuild/meson#3589. Maybe --tag-relative=yes could help to get around the issue.

Copy link
Member

Choose a reason for hiding this comment

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

tags seems to work fine here. I'm not even sure how to test ctags (it's used by vi, right?). Please open a new issue with more details, otherwise this will get lost.

evverx added a commit to evverx/systemd that referenced this pull request May 18, 2018
@evverx
meson: use run_target for generating tags with ctags
cb39773 
In systemd#6561, `run_target`
was changed to `custom_target`, which inadvertenly caused
relative paths to be passed to ctags due to
mesonbuild/meson#3589.
The switch to `run_target` causes absolute paths to be
passed again and makes it easier to jump from file to
file, hopefully delaying the need to exit Vim :-)
evverx added a commit to evverx/systemd that referenced this pull request May 18, 2018
@evverx
meson: use run_target for generating tags with ctags
64c11b2 
In systemd#6561, `run_target`
was changed to `custom_target`, which inadvertently caused
relative paths to be passed to ctags due to
mesonbuild/meson#3589.
The switch to `run_target` causes absolute paths to be
passed again and makes it easier to jump from file to
file, hopefully delaying the need to exit Vim :-)
@evverx evverx mentioned this pull request May 18, 2018
Merged
keszybz pushed a commit that referenced this pull request May 19, 2018
@evverx @keszybz
meson: use run_target for generating tags with ctags
2f09974 
In #6561, `run_target`
was changed to `custom_target`, which inadvertently caused
relative paths to be passed to ctags due to
mesonbuild/meson#3589.
The switch to `run_target` causes absolute paths to be
passed again and makes it easier to jump from file to
file, hopefully delaying the need to exit Vim :-)
Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Aug 23, 2018
@evverx @Yamakuzure
meson: use run_target for generating tags with ctags
b433da2 
In systemd/systemd#6561, `run_target`
was changed to `custom_target`, which inadvertently caused
relative paths to be passed to ctags due to
mesonbuild/meson#3589.
The switch to `run_target` causes absolute paths to be
passed again and makes it easier to jump from file to
file, hopefully delaying the need to exit Vim :-)
Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Aug 24, 2018
@evverx @Yamakuzure
meson: use run_target for generating tags with ctags
43810cf 
In systemd/systemd#6561, `run_target`
was changed to `custom_target`, which inadvertently caused
relative paths to be passed to ctags due to
mesonbuild/meson#3589.
The switch to `run_target` causes absolute paths to be
passed again and makes it easier to jump from file to
file, hopefully delaying the need to exit Vim :-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keszybz keszybz keszybz left review comments

@evverx evverx evverx left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@userwithuid @keszybz @evverx