-D allows for using zero for timestamps and UIDs/GIDs.
-U allows for using actual timestamps and UIDs/GIDs.
Depends on D84206.
Would changing the default behavior to being non-deterministic be a problem? This, non-deterministic behavior, is the default behavior of cctools' libtool and so I thought of replicating that here.
I have added the docs now, thanks!
In binutils, there is a configure-time option --enable-deterministic-archives. For llvm-objcopy and llvm-ar, we find that deterministic builds tend to make more sense so we simply default to determinism. There probably should be a survey whether cctool libtool users really want the default non-deterministic behavior.
I don't know whether it would be a problem or not, but it does make comparing archives harder. That being said, in our downstream llvm-ar, we've had to override the default to use non-deterministic timestamps, because our build system relies on the timestamps being accurate, so that might be evidence that changing it is good. I don't know if it's important to people outside our environment though - there aren't lots of people complaining about the current upstream llvm-ar behaviour, so matching that might make more sense. I don't really have an opinion either way. I'm happy for users of the tool to provide guidance.
It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?
I think if we have Deterministic set to a different default value to libtool, we should definitely provide a -U option to allow a fallback, in case there are people who need that behaviour.
I have updated the diff below to add support for both -D and -U options. Although, the -D flag is not necessary, we perhaps need it to align the behavior with cctools' libtool? I can update it by removing the D flag if people prefer that? Thanks.
Change "timestamps/etc." to simply "timestamps". You're next paragraph explains the bit about other fields, so the "etc" is somewhat misleading/confusing.
Also, combine these into one sentence and add a couple of missing "the"s: "This test checks that timestamps are set to 0 by default or when the -D option is specified, and that they are preserved when the -U option is specified."
I'm not particularly a fan of adding a global variable for this. Are there going to be other configuration options? If so, I'd recommend creating a Config class/struct, that can be created and passed around instead.
(Also, missing trailing full stop in comment)
This function does more than just check the options - it also sets behaviour, based on options that are parsed. I think you need to rename the function to clarify. If you add a Config class, perhaps it could become: static Expected<Config> parseCommandline() instead, and also do the cl::ParseCommandLineOptions call. What do you think?
For now, there aren't any other configuration options that vary depending on the flags passed in on the command line. However, I can still wrap this in a Config class/struct just in case one is needed in the future? What do you think?
Yup, I agree on the name and cl::ParseCommandLineOptions call, I have changed it accordingly now. (I can change the signature too if we decide on adding the Config class - please take a look at my comment above for that).
By "For now" I am referring to the period until we add full support for creating static libraries. ( I am not completely sure yet about the dynamic case)
I'm talking about in the future. I'm guessing there will be other configuration options added in due course, but you have a better idea of this than me. If there are, I'd recommend my suggested approach.
How about Deterministic = true (or false, whichever is the preferred default)?
Config is likely to grow, so it probably is best to pass it as a const &, here and everywhere else.
Consider this instead, combined with the default initialiser:
else if (NonDeterminisitcOption) C.Deterministic = false;