-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow for different logfile encodings #76
Comments
@rcalvo12 thanks! It's probably good to add a unit test. Maybe we could try to add an input in |
@jmshapir, I did some more digging this weekend and a little this morning. As I had seaid in our offline conversation, I was unable to reproduce the error locally. Going back to the project which first prompted this issue, I realized that even in the original case the error does not occur locally. The error is only introduced when I try to run the original version of the builder on FASRC. This presents a problem in that unit tests will only catch that it is working properly if the unit test is done on FASRC. Given this, do you think it might be better to put this issue aside for now and not merging this change? We now have this issue and branch publicly documenting the problem and the workaround in case future users encounter similar issues on their cluster systems. |
@rcalvo12 thanks for looking into this! I agree with your conclusion. We are only aware of one occurrence of the issue and we cannot reproduce it on other architectures. To me that says that a universal fix is likely to have a cost (in terms of maintenance/clarity) that is higher than its benefit. I suggest we do as you say, close this issue with a stable link to the state of the code in the issue branch as of the closure, and delete the branch. If the issue recurs or we hear about it from other users, we can always revisit and base our solution off of the fix implemented on the branch here. |
Thanks @jmshapir! I will wrap this up shortly. |
Summary: In this issue, we set out to patch an issue which had occurred in another project repository. However, we were unable to reproduce the error locally and after further investigation it seems like the error is an idiosyncratic issue that only appears one dataset and only on FASRC. Given the lack of reproducibility we decided in #76 (comment) to go ahead and close the issue without merging. Final state of the issue branch here. |
If the sconscript.log file has characters that are not in "utf-8", scons breaks. This is particularly a problem when parallelizing the build. A simple fix would be allowing the builder to try again with "latin1" if it doesn't read "utf-8" on the first pass.
The text was updated successfully, but these errors were encountered: