-
-
Notifications
You must be signed in to change notification settings - Fork 618
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
Do not write file in io.writefile
if contents hasn't changed
#2137
Conversation
Only after writing out the PR and looking at docs, I've noticed that apparently Therefore I ask maintainers what should be done about it, it seems to be a bit out of place to have that separately in I see that |
I could see an argument being made to have a function in I/O that calls this native function. The I/O lib is just an extension of the Lua IO namespace. There are a few file related functions in the OS namespace.
If you think this is a bug, I would say change this PR to fix that and expose it in the I/O namespace, rather than modifying the |
It could fall into that category, given it could technically cause race condition between the two openes and closes of handle (during which some other process could change the contents). In any case I think it's currently not ideal and not really better in any way than pure-Lua solution. So I definitely think the two options are to either improve this or remove it altogether.
While I see the point, at the same time I'm not sure if I can imagine reasonable scenarios in which some workflow would be broken by that... (The change should be clearly documented though if this was made, to resolve any confusion someone could possibly have)
So to have I'm not 100% sure here though, having just a single |
The native function would be faster than the Lua solution, so that is a downside. We want file IO to be relatively fast. Removal isn't an option, as it would break existing code.
Hyrum's Law comes into play here. While we make no guarantees in our API about file modification if the contents are equivalent, there are likely people relying on this behavior (just given the number of users of Premake).
Again, I'm not convinced that the correct approach at this time is to change underlying behavior of a well established function. I could be convinced to add another, optional parameter that allows you to to an equality check before writing, instead of adding a separate function. |
Sorry, pretty late to the party, but I mostly agree with this. However, the native code for |
I'm going to close this off as this function being changed in this way is not desired. |
What does this PR do?
io.writefile
utility function always writes the file it's been given, even if the contents didn't change.This is problematic for build systems that check file timestamps in order to determine whether a particular file and whatever depends on it should be rebuilt.
In my particular case, there was a premake build step ran before every build to generate primary version file. But to do so, premake first had to run to parse all workspaces, and scripts in there would generate some files within project on their own (transforming
.h.in
to.h
etc.). This unfortunately caused parts of these dependent projects to rebuild every single time, despite no changes in them.How does this PR change Premake's behavior?
No breaking changes. Files will still be written if different or not existing yet. It will help build systems to rebuild less files when not needed.
Besides, there is no reason that I see to write a file and update its timestamp on drive if the contents hasn't changed, thus I didn't provide an option to forcefully write it out. If someone really wants that, they can use
io.open
manually...Anything else we should know?
This is consistent with main logic for generating project files: files won't get written to drive if their contents hasn't changed. So this PR should make this behavior more consistent for if someone uses
io.writefile
function somewhere. Plus it makes for cleaner code than if someone was to check for this manually.I was considering using
a+b
file mode, then seek to beginning to read the file, in order to avoid creating two file handles in the process. Unfortunately I'm not aware of a way for Lua to truncate the file then to write out the new contents. So the current way I think is better, simpler, more readable...Did you check all the boxes?
closes #XXXX
in comment to auto-close issue when PR is merged)You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!