sprtf library should work cross platform
#604
Comments
|
@balthisar I have been using my little I have not yet had time to fully check whether In As advised, will help where I can... thanks... |
|
Candidate #626. |
|
Another one closed! On a roll... |
|
@balthisar yes, you are certainly On a roll... ;=)) So fast in fact that I did not have time to respond on this... My concern is that all you have done mainly is to remove the On many that is fine because they have a second define, like But that leaves many with just But this is not the case in linux. If the user does not add I am just afraid linux users could end up seeing all this I have not thought out a solution yet... and maybe my concern is unwarranted... but I would certainly feel better about the macro being changed to something that the user has to opt-in to use... not get And maybe there is a case for also being able to enable it in other than the Windows Debug build... that is none of it depends just on Will give this some thought, and maybe you have an idea... thanks... |
|
@geoffmcl, good point. It wasn't an issue on Ubuntu, but that's admittedly a small sample size. Perhaps a new define controlled in tidyplatform and cmakelists? |
|
@balthisar not sure what test you did in Ubuntu, but as predicted, if I just do - That is without the option But could not find the And if I turn on Thinking more about it, maybe replace all, well most - there may be some exceptions - need to check - I would also add it as an As to whether there should be some default in say Maybe it could again be based on Still pondering, and experimenting... but feel something should be urgently done for the *nix builds, before we get the expect WTF message ;=)) What would be your idea? Thanks... |
|
I usually test with release, and given our build instructions don't mention that, I think you're right! Let me push a hot fix in the next 45 minutes or so, since I screwed it up.
Yup, this is what I'm going to do. |
|
@balthisar certainly agree with the hot fix using And check the And maybe adjust the |
|
Yeah, the Before: After : stuff, Line 325 should also be changed to PS: Will shortly be |
|
The "hot fix" was fairly easy, in that I simply tweak NDEBUG manually. This fixes code; will go back with more time and implement DEBUG_TIDY or similar. |
|
Okay, I've added ENABLE_DEBUG_LOG as a substitute, and it's live. Preserves behavior on Windows, and no nasty surprises when others are following our build instructions. |
|
@balthisar thanks for the hot fix, but nope! Now I get ENABLE_DEBUG_LOG enabled all-the-time in Windows... Still checking why, but it seems CMAKE_BUILD_TYPE is not defined when I build in windows, so now the CMakeLists.txt enables it all the time... I do not define it when I run So I think preserving my Windows behaviour, to get logging for the Debug build only, must be done outside cmake... Still looking for HOW! |
|
@geoffmcl, huh? Does Right now it's a Windows-specific behavior; it would be trivial to make it so that all platforms require Sorry, thought I was trying to preserve your preference... |
|
@balthisar yes, understand what you were trying to do, and thanks... but, in general, cmake can only define global macros. Not macros that change depending on the build configuration... As stated I run cmake without setting CMAKE_BUILD_TYPE, eg - So agree, the option ENABLE_DEBUG_LOG must be default off for all platforms... please push this if you get a chance... It may be difficult to have this automatic difference in Windows, where Debug and Release are always built by me... Maybe something like, in tidyplatform.h - Or something... still searching... ideas welcome... thanks... |
|
Oh! I see! I consider myself "schooled" on this point. Although I'm fine with
Given that I config, then make, I'm not sure where |
|
@balthisar WOW, out of the Now you have defaulted the cmake And as an aside, I wish the cmake And that points out another problem, now you have added I just wish we were experimenting with all this in a Ok, we have to separate the two ports - unix versus windows. Of course they are, using the default cmake generator, very different! In unix, a standard make Windows ONLYI am sure you understand some of this, and really sorry for the repetition, but it seems important to exactly establish the situation... My 'standard' cmake command line is - But remember that And by default, the Now the actual OR you can use a command line, like I do... which runs the And the Unix/LinuxThe above is very Although, as in Windows cmake, there are some 14 or so generators, only one of which is And maybe it only has two(2) - Release or Debug... and as I understand it, even the Now in a quick test, it seems The actual And it is still up-for-grabs which is built if you do not add that option... SummarySo, yes, I would like to maintain the default I have had for YEARS, but that is not the important issue... I will continue to search for that... but only when all builds, in all systems settle back to what we had... Namely, a default build in all, using the minimum of AND that should cover all possible generators available in that system... So back here, late at night, pondering this... the single important issue seems to be to default that option Will need some time, and help to address other items... as always, ideas welcome... thanks... |
Wait, WTF???? Yes, it should be OFF; did I screw up a local merge???
Yes, thank you!!! I will digest everything else... |
|
@geoffmcl, thanks for the in-depth explanation of the Windows build process. When I'm in the Windows world, I've actually built Our README indicates this: ... which really mislead me into thinking it was similar to the Unix way. I actually kind of like the fact that you can configure once, and build multiple targets (actually, I get that in Xcode, but not when building with I will study this issue in-depth before making any more changes; it's a lot more complex than it seemed at first. |
|
@balthisar on further reflection, after a good nights sleep, while I was initially all for expanding this DEBUG capability to Now, if you turn on So maybe Need to reflect more... I am a slow thinker, and want to thoroughly test... of course, I would move this testing back to a branch, maybe While we agree, the What are your thoughts, or the thoughts of others on this? Thanks... Concerning the README/BUILD.md docs, as always, I agree this could do with some enhancements. Ideas welcome... And then there is the question of the That's an easy change in For windows I chose a local to the runtime directory. Again that is very convenient for running in the MSVC IDE Debugger, which uses the solution directory by default, and as you will see the root But this is not the usual place, or name, for Again, look forward to further comments? Thanks... And as a separate issue, just built such a debug tidy, 5.5.63, with symbols as well, and could not find an Tried to run it using I have tried And perhaps others with more What am I doing wrong? Thanks... |
|
@geoffmcl, oh, what a can of worms I opened! I agree, let's take this to another branch. In the CMakeLists.txt, I think we can set it up so that if the other debug options are enabled, we can infer that ENABLE_DEBUG_LOG is implied. So that's one chink out of the way. As for convenience, here's what I do on macOS: I Given that we're really the only two active developers, I would tend to err on the side of "least surprise," i.e., if someone follows our build instructions, they get a working Tidy without debug messages. I'm certain that you agree. So, then, how to we simply enable debug output for people who make a deliberate choice to contribute code, while at the same time not handicapping our own workflows (which is what I'm afraid I did to you)? If someone goes to great pains to enable debug output in a Release build, I don't see the point of stopping them; it's a deliberate act. It would be nice to prevent it, but I don't see it as a priority. Default name, we could get from the version number; might be convenient to you if you add the RC_NUMBER field. Otherwise, I'm ambivalent. For me, the runtime directory works great for make or Xcode. With make, it's in the build directory, and with Xcode it's in the out-of-source-build directory, exactly where I expect it. I'll try to deep dive this on the Mac/Linux front! |
|
@balthisar have not had time to explore all here, but think I have at least found why the log file is not written in Ubuntu... As part of commit dedcb7b, the definition of SPRTF was removed from diff --git a/src/sprtf.h b/src/sprtf.h
index 14c3e02..c589707 100644
--- a/src/sprtf.h
+++ b/src/sprtf.h
@@ -62,10 +62,6 @@ TIDY_EXPORT char *get_date_time_stg();
TIDY_EXPORT int gettimeofday(struct timeval *tp, void *tzp);
#endif
-#ifndef SPRTF
-# define SPRTF sprtf
-#endif
-
#ifdef __cplusplus
}
#endifWithout this, in other places And agreed, it is trivial to only accept the additional debug options, if the primary is on, or at least warn and/or advise that otherwise say if And I do not particularly want to somehow stop enabling the extra debug in the The only thing I would like is to be able to add it only in the Debug configuration, by default if possible, IFF MSVC... as I had, but as stated, not important, and will work on that... The main thing now is to even get the log file written, in all ports, if Will try to work on solutions here, as time permits, but would be great if you beat me to it... thanks... |
|
@balthisar well to be very frank it is not Now I see more potential problems! You have included They will include If we do want to add SPRTF to the distribution, and I suggest not, then at the very least In essence, we should never be distributing a I do not mind the restriction that To me the only use of this extra debug stuff, is if you are DEBUGGING a potential problem in libtidy, before it is distributed... and really helped me recently to track down the problem #597, and several other items before this... it is also a learning/teaching tool... thus My fixes would include essentially moving Before I start making fixes, in perhaps an |
|
@balthisar pushed initial cut to Other testing welcome... thanks... |
|
Have now pushed another small fix to the Note, have added back the auto-building of Have now further tested it in Ubuntu 14.04, and in Raspbian GNU/Linux 8.0 (jessie), and all appears to work fine... Conveniently, issue #631, @jokester PR #630 provides a convenient memory leak test. I have a perl script, Thus in a Note it clearly shows the memory leaks... real easy... Now I would/could share the So this has now been tested in Windows 10, Ubuntu and Raspbian, and all appears ok... created PR #632... If others could review/test would merge that PR... thanks... |
|
@geoffmcl, the |
|
@balthisar well they were always there, just under an As discussed elsewhere it is not really possible to switch things according to the MSVC build configuration, Debug, Release, etc, at the cmake level... and I only recently got my auto-special-debug back working as I had, and would like to keep that, if possible... What is the notice you are seeing? The Maybe the original macro could be changed to What are you suggesting? Thanks... |
Nothing, at this point. Given that everything works on multiple platforms, and it looks like everyone finds the behavior acceptable, I'd vote for closing this. |
|
@balthisar although still thinking about adding an Accordingly, agree, seems this can be closed... |
Although this is intended for debugging, its features should be platform agnostic. It doesn't affect release builds of Tidy, so this isn't a hard milestone.
The text was updated successfully, but these errors were encountered: