任务单 #43995

generate_packets.py get run on every build

开放日期: 2022-03-01 17:49 最后更新: 2022-04-14 19:43

报告人:
属主:
类型:
状态:
关闭
组件:
里程碑:
优先:
5 - Medium
严重性:
5 - Medium
处理结果:
Fixed
文件:
2

Details

Current master: even when I run 'make' twice in a row, without changing absolutely anything, the output shows that generate_packets.py gets run. Don't understand why. I've checked timestamps of both generate_packets.py and packets.def, and neither has any weird in-future timestamp.

This is probably mostly harmless in a development branch, but if the same happens (haven't tested) in a build from a release tarball, that breaks a build without python.

任务单历史 (3/13 Histories)

2022-03-01 17:49 Updated by: cazfi
  • New Ticket "generate_packets.py get run on every build" created
2022-03-01 18:05 Updated by: cazfi
评论

This is likely because of #43953 -> timestamp of the generated files is not updated, so it remains older than that of source files, no matter how many times generator is run.

2022-03-01 19:38 Updated by: alienvalkyrie
评论

One (quick and dirty) way to solve the potential problem for release tarballs would be to run the script with --force-overwrite once, to make sure the generated files are newer. For this, it might be sensible to make some of generate_packets.py's parameters more directly controllable through configure options. It might even be sensible to make force-overwrite the default, and only enable lazy-overwrite with certain configure options.

Another (more general) solution to prevent re-running the script every time, even with lazy-overwrite (which could become a problem anyway if the script ever starts taking longer to run), would be to take the intermediate dummy packets_generate file (which is currently temporarily created to guard against problems in parallel builds) and make it a permanent resident to use as a timestamp for when generate_packets.py was last run – it would always get refreshed by the build system, even when the actual output files didn't change.

Also, for the potential release tarball problem – maybe there could be a --without-python configure option (that must, of course, check that all generated files are already present) to generally sidestep that problem, rather than just trusting that the build system won't be running any Python scripts?

2022-03-01 19:46 Updated by: cazfi
评论

Reply To alienvalkyrie

Another (more general) solution to prevent re-running the script every time, even with lazy-overwrite (which could become a problem anyway if the script ever starts taking longer to run), would be to take the intermediate dummy packets_generate file (which is currently temporarily created to guard against problems in parallel builds) and make it a permanent resident to use as a timestamp for when generate_packets.py was last run – it would always get refreshed by the build system, even when the actual output files didn't change.

For that to work, it should also be created to the source directory. Which in itself might even be an improvement (that in case of multiple build directories, they all look at the same 'packets_generate')

But my initial thought was option (3) that even with lazy-overwrite we would still refresh the timestamp (touch the generated files).

2022-03-01 20:00 Updated by: alienvalkyrie
评论

Reply To cazfi

But my initial thought was option (3) that even with lazy-overwrite we would still refresh the timestamp (touch the generated files).

At that point, lazy-overwrite would do essentially nothing (except leave the originals untouched when there's an error in the generation process, in which case the build fails anyway, so it barely matters). The whole point of lazy-overwrite – at least for me – is that the generated files, particularly the headers, don't get touched, to avoid having to rebuild approximately the entire codebase. Especially right now, when I'm refactoring the generation script (which should leave the generated files unchanged), that saves a lot of time.

(Edited, 2022-03-01 20:00 Updated by: alienvalkyrie)
2022-03-01 20:23 Updated by: cazfi
评论

Reply To alienvalkyrie

Especially right now, when I'm refactoring the generation script (which should leave the generated files unchanged), that saves a lot of time.

If we consider that to be the special case -> one where one needs to explicitly set (non-default) options, how about:

1) Make forced overwrite the default in generate_packets.py, requiring extra option ("--lazy-overwrite") to change
2) Add support to common/Makefile (and meson.build?) for user to pass *additional* parameters to generate_packets.py, likely via environment variables

2022-03-02 00:40 Updated by: alienvalkyrie
评论

I do think it might be sensible to have lazy-overwrite enabled by default – not rebuilding stuff that hasn't actually changed is reasonable IMO. And since the various refactoring changes will end up in master one by one, this doesn't only affect people working on the generation scripts in particular, but anyone regularly building from master.

I also do think, whether lazy-overwrite is the default or optional, it'd be sensible to have an unambiguous indicator that current generated files are up to date, regardless of their timestamp (e.g. in the form of a dummy file). (Note that the same can be said for utility/generate_specenum.py and any other future code generation scripts that might get added.)

2022-04-11 06:59 Updated by: alienvalkyrie
评论

Reply To cazfi

2) Add support to common/Makefile (and meson.build?) for user to pass *additional* parameters to generate_packets.py, likely via environment variables

While looking at this (and trying to understand how autoconf works), I just noticed configure.ac already does AC_SUBST([GENERATE_PACKETS_ARGS]) – the variable just isn't used anywhere yet.

(Edited, 2022-04-11 07:00 Updated by: alienvalkyrie)
2022-04-11 07:14 Updated by: alienvalkyrie
评论

Reply To cazfi

1) Make forced overwrite the default in generate_packets.py, requiring extra option ("--lazy-overwrite") to change
2) Add support to common/Makefile (and meson.build?) for user to pass *additional* parameters to generate_packets.py, likely via environment variables

Attached patch does exactly that (minus the meson.build part). Part of me wants to split it into two even tinier patches, since they aren't really dependent on each other.

The GENERATE_PACKETS_ARGS variable isn't documented in ./configure --help – should we do that? If I understand autoconf correctly, that would mean replacing the AC_SUBST with an AC_ARG_VAR macro (plus description)?

(Edited, 2022-04-11 09:01 Updated by: alienvalkyrie)
2022-04-13 07:08 Updated by: alienvalkyrie
  • 属主 Update from (无) to alienvalkyrie
  • 处理结果 Update from to Accepted
评论

Reply To alienvalkyrie

Reply To cazfi

1) Make forced overwrite the default in generate_packets.py, requiring extra option ("--lazy-overwrite") to change
2) Add support to common/Makefile (and meson.build?) for user to pass *additional* parameters to generate_packets.py, likely via environment variables

Attached patch does exactly that (minus the meson.build part). Part of me wants to split it into two even tinier patches, since they aren't really dependent on each other.

The GENERATE_PACKETS_ARGS variable isn't documented in ./configure --help – should we do that? If I understand autoconf correctly, that would mean replacing the AC_SUBST with an AC_ARG_VAR macro (plus description)?

New patch includes documentation for autotools build, and support for additional packet gen arguments in meson.build – the latter is completely untested (except that it didn't break the CI build).

While "disable lazy-overwrite unless specifically requested" isn't my preferred way or resolving this issue (which would've been adding dummy files as unambiguous indicators of last regeneration), I think it's reasonable enough (and simpler).

(There is still an argument to be made to split this into "pass through additional arguments" and "disable lazy-overwrite by default".)

2022-04-14 19:43 Updated by: alienvalkyrie
  • 状态 Update from 开启 to 关闭
  • 处理结果 Update from Accepted to Fixed

编辑

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » 登录名