Cleanup #56
Cleanup #56
Conversation
I attempted to only clean out code that was not apparent as to why it shoudl ever be added back in. Code that was well commented as to why it was dead was left intact. That said, there may be stuff I cleaned out that might be useful as a reference. Otherwise dead code has a nasty habit of developing bitrot, which I observed in a few of these cases...
Plus a couple of dead code excisions.
While I'm a fan of tabs, it's either spaces or tabs, never both in a hodgepodge mix like this had.
How I cleaned up:
find InWorldz OpenSim -iname '*.cs' -exec sed -i 's/\t/ /g' '{}' \;
find InWorldz OpenSim -iname '*.cs' -exec sed -i 's/\t/ /g' '{}' \;
Only the really easy ones, some of the more difficult are to be attacked in a later patch.
Now everything references Halcyon.ini, making things much more consistent.
…pt away. This is the most complex change so far in this process. For these changes the VersionInfo file was just the ticket, and that was easy, just needed some tuning. llGetEnv did get some potentially backwards compatibility breaking changes however - see Issue IslandzVW#51 for the place to discuss those. I also cleaned out a lot of local duplication and unneeded function-call passing of the version string. In one location I may have broken ownership rules by giving Phlox direct access to VersionInfo, I'm not 100% certain. Overall I attempted to try and keep the goals of each of these version and product name calls the same, just centralizing where they got that information from. The main idea being that the more direct the access, the easier it is to determine what the effects are for any given change. For example, previously it was a bear to find all the places changing the version string format would effect, now it's as simple as looking at where VersionInfo is used.
|
Wowsers. Give me some time to review this one :) |
|
We're provisionally merging this. The last commit contains some stuff that could potentially break scripts checking for an InWorldz environment. We're going to need to be able to override that for the IW grid, so @jimtarber @kf6kjg and myself should have a discussion about where best to put the override for backwards compat. |
|
I'm slowly working through this one too. There's a lot to cover in this commit. Overall I'm pleased; it's nice to see this kind of cleanup of old stuff and lesser source code issues (especially things like hard tabs, etc). But some of the changes are fairly sweeping, and I do have some concerns. First, I do want to point out that "Initialise" and "Normalise" are actually spelled correctly in England, Canada and just about everywhere outside of the USA. Just changing that one spelling ("Initialize") to a different spelling resulted in most of the size of this commit, which seems to me to be mostly unnecessary work. Please consider the cost/benefit potential of not fixing spelling errors in interface names going forward. (That said, I prefer the 'z' versions myself for these, even though I'm in Canada, and it's kind of the InWorldz namesake spelling anyway. At InWorldz, we're kinda known for changing 's' to 'z' in overzealous ways.) 😜 And I think the spelling fix for AuthenticateAsSystemUser is a good one, so a bit of a balancing act there depending on impact. The change to the DATA_SIM_RELEASE option in llRequestSimulatorData is very similar to the llGetEnv("InWorldz") issue. Again, some software may be using that to distinguish grids, especially in the absence of a "gridnick" option. I don't think we can just change it to return the software name, at least on InWorldz grids. Similarly, if the user-agent string in HTTP requests (elsewhere0 is changed from InWorldz to Halcyon, the string comparison in llSetContentType may not be able to detect calls from Halcyon scripts (see the hard-coded test in llSetContentType, not changed by the commit). I'm also a bit concerned about removing huge chunks of commented-out code (inside #if false tests). That method is sometimes used to put a cap on work in progress, to allow it to be stashed in source control, not lost in a branch, but to ensure that the untested partial code has no effect on the production servers. If you were the one to comment it out, of course that's different. But a lot of code has been removed from RemoteAdmin.cs, which I believe @mdickson was working on some time ago before having to switch gears to materials and other priorities. Similarly for large blocks of disabled code in AssetCapsModule.cs. Both may also just be very old OpenSim code that we don't use, and can come out, but I think it's important to know for sure that it's not a work in progress before those kinds of significant deletes. Also sometimes code is disabled but left in place to document a conscious decision to not include it. This helps to avoid someone who knows it's missing from coming along and adding it back (this time not commented out). A good example of that is the disabled support for estate_id and parent_estate_id in EstateSettings.cs (handleIncomingConfiguration). We're specifically disabling those two incoming fields so that the viewer user can't just change the estate IDs to something they make up. Removing that commented-out code puts that operation at risk of being "fixed" down the road. (It also hides the well-known implementation while it is present there but disabled, so that if we ever decide that we do in fact want to do something with it, we have to dig to even find out what the configuration key names are that we're intentionally ignoring. The calculation of SimWideMaxPrims in SendLandProperties is another example of that, where we chose to put an explicit prim count in the region.xml file, but in the absence of that, this disabled code is how it would be calculated (based on land). If we ever decided to support var regions or anything like that, we might need that (disabled) code. |
|
Still reviewing the changes, but did you check to see that the caller of RESTInterregionComms.cs DoAgentDelete() won't gag on the modified "str_response_string" that now has "Halcyon" in the text? That's a protocol change, and it's probably safe (the caller is probably checking the status code if anything, but it is a change to the protocol that needs to be confirmed to be safe. |
|
I tried testing an updated master, and I was having an exception at startup loading the modules. After a short investigation, it seems I had an old InWorldz.WhisperVoice.dll in my bin folder, and because it did not have a PostInitialize (it had PostInitialise), it was generating a load exception. Of course the solution is to delete the DLL, but it highlights the kind of problem that can happen with changes to interfaces, and why they should be made when there is good value to the change. (It did not cost much time; the exception report indicated PostInitialize in one of the exception fields and so I knew what the problem was right away.) This also was only a developer test problem; our production tools remove all DLLs, EXEs and PDBs on each upgrade, so it wouldn't have been a problem on the main grid either. |
|
Okay, one more issue in the final commit. GridInfoService.cs changes the constant "platform" value from "OpenSim" to whatever the software name is ("Halcyon Server"). This is the data directly returned to the viewer on a call to get the Grid Info. I'm not sure in what cases a viewer behaves differently when the platform is "OpenSim", and that may depend on the viewer. So this is also a protocol change, and probably not a safe change. To go ahead with that one, we'd probably have to check with the IW3 viewer, as well as IW2, Firestorm, Singularity and Cool VL teams at the very least. |
|
I've discussed that last one with the IW3 viewer developer and IW3 tests the URI returned to see if it contains "inworldz.com". Otherwise, e.g. on dev test servers, it only recognizes "OpenSim" as the platform. Which would be a problem for this change, except that it also defaults to "OpenSim" for unrecognized platforms. Also, it is believed that other viewers will also default to "OpenSim" for any unrecognized platform value. So we may be able to change that to indicate Halcyon software, or InWorldz (or conditionally one or the other based on that InWorldz=1 proposed INI option). But it looks like we could probably proceed with this change. More info: The response from the server includes "gridnick", which is what the viewer calls "grid_login_id" (e.g. "Aditi"), and "gridname", which is what the viewer calls "label" (e.g. "Second Life Beta"), and platform is the underlying software tech (e.g. "secondlife"). So in theory it would be suitable for us to plug "halcyon" in that last one, for all halcyon servers and grids, including InWorldz. However, it seems that platform is typically a single lowercase word, so I'd like us to drop drop the "Server" from that and return just "halcyon". |
|
There are also some other keys recognized in the grid info response, such as "administrator" which is the grid owner (company). For both SL and SL beta, the administrator is "Linden Lab". So for InWorldz grids that should be "InWorldz LLC", but for other grids something else. So we probably should add an admin="InWorldz LLC" to the grid info in the INI file, and let other grids and test grids specify their own. |
|
Good to see an in-depth analysis! I do admit that I cannot see all possible consequences: but that's why we have these reviews! Thank you for the exercise in having to dig through and verify that I did do this correctly and didn't wind up creating unexpected breakage! "Initialise" and "Normalise": I understand that they are "correct" in other countries than the US. However, a search of the Oxford English dictionary in UK English for "initialise" returns "initialize", and the Cambridge English Dictionary does likewise. A random spelling site lists the "ize" variants as archaic in Britain, but current in both US and Canadian English. Our own wiki, admittedly under my editing, states to use "ize", however that was based on a review of the InWorldz-added modules which were using "ize" except when interfacing to the OpenSim modules. (Yeah, I figured I'd have to defend this one at some point! :D I didn't hunt down "Normalize", but I presume it's in a similar state.) DATA_SIM_RELEASE is not LL standard, and it also changed what it returned depending on the test If I read the code correctly, As regards the The only places in the code that read WRT to the InWorldz.WhisperVoice.dll problem, I had that myself - see #54. In regards to the last two comments on further changes, we may need to look into that. I'm likewise recalcitrant to add new stanzas to the INI unless these things are thought out thoroughly. Such may need its own issue report to provide a place for discussion. |
|
Yes, all of this is expected on such a large set of changes and exactly why we have a few eyes reviewing it. Dave reviews all of my changes as well, and I tend to push most commits to a branch and wait for him to review and merge my PRs. (I tend to review many of his as well, although they tend to be more tactical changes, or new code, and thus either required changes or not likely to have any impact.) It's good to have more eyes on something. And I agree many of these things are related and need a more unified strategy and discussion. We're busy with the first production use of Halcyon (crossing problems at the moment), so we'll need to defer that temporarily, but I'd like to get the version/conditionals/etc wrapped up asap to enable further work. |
This is a series of patches designed to just remove some of the dirt in the code. Some things are simple but broad, such as whitespace cleanup and spelling changes or dead code purging, some are more helpful and specific.
In one case however there's a scripting backwards compatibility change: see the comments on 37bcad1 and Issue #51 for more information.
Yes the changes are very big.