Change ISIS3DATA to ISISDATA

[scsides]

Changed build/install references from ISIS3DATA to ISISDATA

Description

Changed build/install references in the cmake, IsisPreferences, IsisTestPreferences from ISIS3DATA to ISISDATA

Related Issue

#3727

Motivation and Context

Part of getting the application specific under revision control

How Has This Been Tested?

The system builds with the changes made so far. However, it does not test. Additional PRs will be made for each mission area to address the failures where the 3 has not been removed.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Documentation change (update to the documentation; no code change)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have read and agree to abide by the Code of Conduct
• I have read the CONTRIBUTING document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

• I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

[KrisBecker]

This looks to me like a "Breaking change" rather than a "New feature". I can say for certain I have many scripts that use this environment variable. Everyones custom IsisPreferences files will break. And, your current installation page (https://github.com/USGS-Astrogeology/ISIS3/blob/dev/README.md#Installation) uses this environment variable to describe to users how to update this data area.

No change to ISIS3TESTDATA?

This all seems rather sudden. Why is this change being made?

[rbeyer]

Well, we're on the path to ISIS 4, ISIS 5, and beyond, it is inevitable that the '3' is going to get scrubbed out of things, so I understand why it is being done, but I agree that this seems like a 'breaking' change to me.

[KrisBecker]

Fair enough. I understand that.

However, there are some pretty serious consequences with this change. No warning/discussion on how to mitigate that into existing (user) environments.

[scsides]

This is a breaking change and all work is being done in a feature branch, not the "dev" branch. The issue, #3727, is a piece toward the discussions in RFC https://github.com/USGS-Astrogeology/ISIS3/wiki/RFC4---Migration-of-ISIS-Data-to-Git . The data area is being scrubbed to get non-large application data (i.e., icons, translation files, ...) under revision control, so kernels, calibration files, ... can be put under LFS revision control.

[rbeyer]

@scsides that sounds like the right technical procedure. Sometimes we're like the blind men with the elephant, and can't see (or remember) the whole picture. Thanks!

I think that @KrisBecker has a good point about telegraphing this to users. While changing the name of the environment variable is a relatively simple change, it may have the potential to 'impact' the average user in a more visceral way than more technically comprehensive changes 'under the hood.' This has been the name of the environment variable for almost two decades and may have all kinds of weird ramifications out in userland. So clear documentation and guides for how to minimize the impact of this change should be high priority when this change gets shipped.

[krlberry]

This looks good to me on a software level. I agree that we need some way to inform users of the change to environment variables before this is ultimately released.

[jlaura]

@rbeyer @KrisBecker This is also something that we are going to discuss at the ISIS TC meeting next week. Specifically, we have external organizational requirements to make changes to the way that we are manage data. We are not in a position to push back.

The RFC went up as soon as we were informed about the changes to data that were required. The item to discuss next week concerns how we manage external requirements/mandates. The RFC process seems to be the best method to communicate these changes to our user community. Additionally, the changes will make it into the change log with the (hopefully) 4.1.0_RC and release. This will also mean that all of the documentation is updated as usual.

I am also going to disagree with @scsides . Our definition of a breaking change is here: https://github.com/USGS-Astrogeology/ISIS3/wiki/RFC2:-Release-Process#versioning. For better or worse (something else to discuss at the TC?), the change to the environment variable would not be classified as breaking. That isn't to say that it should or should not be, just that right now, the way the docs are written it isn't because we are outside what we defined as the API. The definition at the top of the linked page is misleading because it uses the word 'any' while then providing a function signature example.

Thanks too for watching the repo and getting involved in the discussion. Engagement is super appreciated!

[rbeyer]

@jlaura I don't think that we're talking about 'pushing back' against this change, I think we all understand it is inevitable. That's fine.

Fair point, I don't think changing an environment variable causes your major version number to 'tick' so let's not talk about it that way.

The most important point here is that this change to the name of the environment variable has the potential to cause much pain and suffering in the user community, and we all need to work to make sure that we have documents pre-written to point people to when they inevitably have issues, etc.

[jlaura]

@rbeyer Apologies if my post seemed defensive - not intended at all.

I don't feel like anyone is pushing back. I do think that we have an opportunity to discuss the impacts of mandated changes on our side as the part of the contributor team that is hosting the data. This is a great chance to bring this to the TC and discuss how best to go about realizing these changes while still upholding our goal of being transparent and communicating well.

I also agree 100% that documentation, updates to the FAQ, etc. are going to be super important as we make this transition. Luckily, we will have lots of eyes helping to make sure the change is as smooth as possible for our user base. I am sure we will have some good lessons learned after this.

[rbeyer]

@jlaura are you crossing the streams? This issue is about an environment variable name, but I feel like you're talking about the data storage situation. They're related, but they're not the same, right?

[jlaura]

Yes, related, but not the same. It makes sense to handle them both at the same time.

[scsides]

@jlaura is correct about this not being an API breaking change. @KrisBecker has a point, scripts that use ISIS3DATA will need to be modified to work with newer versions that use ISISDATA. PRs into 'dev' will include updated documentation so everyone is informed of the changes, and we will cover as many uses cases as we can to assist users and maintainers with the knowledge they need. The environment variables themselves are set and unset automatically when a version of ISIS is activated/deactivated, so users of installed versions should not need to manually switch.

[KrisBecker]

While I agree this is technically not API breaking as defined by the current policy, it potentially breaks a lot of stuff. It appears this type of change should be covered by policy. Perhaps a period of deprecation could be specified where both environment variables, ISIS3DATA and ISISDATA, refer to the same value to ease the transition and provide an environment to assess the impact of user's environments. It might be a good idea to treat this version as a "breaking change" release.

Here are but a few potential issues I have identified:

ISIS3DATA is hardcode in the ISIS appdata branch

Most of these occurrences are in unit tests, some are in main applications and others are in comments and should probably be changed for consistency. However, in some cases, the change would not be backward compatible (is this a breaking change?). Its likely these issues have already been identified, by here is a list of the occurrences in the appdata branch that directly use $ISIS3DATA:

(isis3/appdata)$ find ./isis/src -name '*.cpp' -exec grep -A1 -C0 -H ISIS3DATA {} \;
./isis/src/voyager/apps/voy2isis/main.cpp:  QString lsk = "$ISIS3DATA/base/kernels/lsk/naif????.tls";
./isis/src/voyager/apps/voy2isis/main.cpp:  QString sclk = "$ISIS3DATA/voyager";
./isis/src/tgo/objs/TgoCassisCamera/TgoCassisDistortionMap.cpp:    // of the CCD. (See $ISIS3DATA/tgo/assets/distortion/DistortionModelA3CorrRoots.jpg).
./isis/src/tgo/objs/TgoCassisCamera/TgoCassisDistortionMap.cpp:    // of the CCD. (See $ISIS3DATA/tgo/assets/distortion/DistortionModelA3DistRoots.jpg).
./isis/src/rosetta/objs/RosettaVirtisCamera/unitTest.cpp:    Cube visCube("$ISIS3DATA/rosetta/testData/V1_00388238556.cub", "r");
./isis/src/rosetta/objs/RosettaVirtisCamera/unitTest.cpp:    Cube irCube("$ISIS3DATA/rosetta/testData/I1_00382172310.cub", "r");
./isis/src/rosetta/apps/rosvirtis2isis/main.cpp:    QString sclk = "$ISIS3DATA/rosetta/kernels/sclk/ROS_??????_STEP.TSC"; 
./isis/src/rosetta/apps/rosvirtis2isis/main.cpp:    QString lsk  = "$ISIS3DATA/base/kernels/lsk/naif????.tls"; 
./isis/src/viking/apps/viknobutter/main.cpp:  QString maskParameter = "$ISIS3DATA/viking" + toString(spn) +
./isis/src/system/apps/dempack/main.cpp:      QString izpackFile = fileList[j].replace("ISIS3DATA", "{ENV[ISIS3DATA]}");
./isis/src/messenger/objs/MdisCamera/MdisCamera.cpp:    // in $ISIS3DATA/messenger/kernels/iak/mdisAddendum???.ti for the
./isis/src/messenger/objs/MdisCamera/MdisCamera.cpp:    // See $ISIS3DATA/messenger/kernels/iak/mdisAddendumXXX.ti or possibly
./isis/src/messenger/objs/MdisCamera/MdisCamera.cpp:    // $ISIS3DATA/messenger/kernels/ik/msgr_mdis_vXXX.ti for the *_OD_K
./isis/src/socet/apps/socetlinescankeywords/main.cpp:  //      of 0.007 mm gotten from $ISIS3DATA/mgs/kernels/ik/moc20.ti.  (The
./isis/src/newhorizons/apps/mvic2isis/main.cpp:  QString lsk = "$ISIS3DATA/base/kernels/lsk/naif????.tls";
./isis/src/newhorizons/apps/mvic2isis/main.cpp:  QString sclk = "$ISIS3DATA/newhorizons/kernels/sclk/new_horizons_???.tsc";
./isis/src/control/apps/cnetstats/cnetstats.cpp:        Pvl pvlTemplate("$ISIS3DATA/base/templates/cnetstats/cnetstats.def");
./isis/src/control/apps/cnetref/main.cpp:        pvlTemplate = Pvl("$ISIS3DATA/base/templates/cnetref/cnetref_operator.def");
./isis/src/control/apps/cnetref/main.cpp:        pvlTemplate = Pvl("$ISIS3DATA/base/templates/cnetref/cnetref_nooperator.def");
./isis/src/control/apps/cnetedit/main.cpp:      Pvl pvlTemplate("$ISIS3DATA/base/templates/cnet_validmeasure/validmeasure.def");
./isis/src/base/objs/Pipeline/unitTest.cpp:  ui.PutFileName("FROM",  "$ISIS3DATA/odyssey/testData/I00831002RDR.even.cub");
./isis/src/base/objs/Pipeline/unitTest.cpp:  ui.PutFileName("FROM2", "$ISIS3DATA/odyssey/testData/I00831002RDR.odd.cub");
./isis/src/base/objs/Pipeline/unitTest.cpp:  ui.PutAsString("FROM", "$ISIS3DATA/odyssey/testData/I00831002RDR.cub");
./isis/src/base/objs/Pipeline/unitTest.cpp:  pc1.SetInputFile(FileName("$ISIS3DATA/mro/testData/PSP_001446_1790_BG12_0.cub"));
./isis/src/base/objs/Pipeline/unitTest.cpp:  pc2.SetInputFile(FileName("$ISIS3DATA/mro/testData/PSP_001446_1790_BG12_0.cub"));
./isis/src/base/objs/EmbreeTargetManager/unitTest.cpp:    QString copyDSKFile = "$ISIS3DATA/base/testData/hay_a_amica_5_itokawashape_v1_0_64q.bds";
./isis/src/base/objs/ShapeModelFactory/unitTest.cpp:    QString inputFile = "$ISIS3DATA/mgs/testData/ab102401.cub";
./isis/src/base/objs/ShapeModelFactory/unitTest.cpp:    inputFile = "$ISIS3DATA/galileo/testData/1213r.cub";
./isis/src/base/objs/ShapeModelFactory/unitTest.cpp:    // inputFile = "$ISIS3DATA/;
./isis/src/base/objs/Sensor/unitTest.cpp:                        "$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub");
./isis/src/base/objs/Sensor/unitTest.cpp:            "$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub" << endl;
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Gruen/Gruen.cpp:   * @brief Load default Gruen parameter file in $ISIS3DATA/base/templates
./isis/src/base/objs/Gui/GuiFilenameParameter.cpp:    QString file = FileName("$ISIS3DATA/base/icons/view_tree.png").expanded();
./isis/src/base/objs/Application/Application.cpp:    QString env6 = "printenv ISIS3DATA >> " + tempFile;

Example of ISIS3DATA in Code

Here is an example of ISIS3DATA in the API. One could still argue this is not API breaking, but its clear the first example is not backward compatible. The second one can be made so by replacing the occurances of $ISIS3DATA/ with a $.

By the way, I am not a big fan of renaming all the main applications to main.cpp because in some GUI IDEs, its difficult to tell which main application you are editing. I am a SlickEdit user. Which main.cpp tab goes with which source file? Visual aids requires a significant amount of screen real estate. Likely this makes things easier to turn ISIS mains into callable functions, but it makes some development tasks that were easy and straightforward much more difficult/inconvenient/frustrating/annoying.

Kernel File Management Scripts

Eventually, scripts that manage NAIF kernels in the ISIS environment refer to the install directory. There are many mission kernel download/install scripts that use the $ISIS3DATA environment variable to navigate to the appropriate data directory and generate the kernel DB file, typically with a ./makedb command. These type of scripting scenarios will present challenges as they will require modifications. They will not be backward compatible without additional consideration of explicit support for both variables. I know USGS maintains many of these types of scripts that will require modifications. Our kernel maintenance scripts will require these modifications as well.

Pipeline Processing Scripts

There is likely significant development of ISIS processing scripts that makes use of $ISIS3DATA that will have to change. While there are ways (i.e., IsisPreferences) to specify ISIS data directories, its likely many are not using this overloaded form (e.g., $rosetta/kernels/dsk/ROS_CG_M004_OSPGDLR_U_V1.bds) of file specification in scripts. Unfortunately, this form of ISIS file referencing does not translate into the Unix Shell without use of the $ISIS3DATA environment variable. Changes will still be required that are not readily backward compatible. This will initially make regression testing of previous versions challenging.

Legacy Use

Note that the ISIS rsync servers has an isis3data module that serve up the ancillary data for ISIS3. This will soon be obsolete, but what happens to ISIS2 on the servers? Note that ISIS2 is no longer maintained, but USGS/Astro is still providing the system through the the rsync server.

And I'm gonna say it - ISIS2 uses the $ISISDATA environment variable. That was the main reason for creating the ISIS3DATA environment variable in ISIS3 - to avoid conflict. This issue will likely serve as the opportunity to wipe ISIS2 off the internet. Its probably time for it, but it needs to be addressed.

IsisPreferences

Perhaps not many users do this, but I always have a copy of IsisPreferences in $HOME/.Isis. (MacTip: I always set GuiHelpBrowser = open so that your user preferred browser is used.) And while this is not recommended practice, I retain much of the contents from the original $ISISROOT/IsisPreferences file including the mission directory specifications that use the $ISIS3DATA environment variable. This will lead to breakage and would likely be difficult for users to determine why they are getting missing file errors.

Summary

This is going to be a potentially painful change for many users, particularly for users who have a long history and investment in ISIS. While the current API definition of "breaking change" may not apply here, it is another form of breakage that should be covered by policy. To ease the transition, it would be nice to provide a period of depreciation of the ISIS3DATA environment variable.

I am not saying this should not be done. Changes like this are necessary/inevitable/painful, but I think we can and should do more to ease the transition.

Thank you for your consideration...

[jlaura]

@KrisBecker Can you please add this discussion associated with the RFC? Comments on merged PRs are not really linked from anywhere and will get lost.

#3727

Thanks!

[KrisBecker]

@jlaura ok, done - I think. Perhaps there is a better way to do this rather than copy/paste?

[jlaura]

I don't think I can migrate a single post out of a PR into an already opened issue and I wanted to make sure that you retained authorship on the move.

Thanks for moving the comments!

[oleg-alexandrov]

As I see it when I run spiceinit with ISIS 4.1.0, it expects ISISDATA, but the doc at

https://isis.astrogeology.usgs.gov/documents/InstallGuide/index.html

still says ISIS3DATA. Setting both to the same value fixes things. Also spiceinit web = true is broken now, but this is a different discussion.

[jessemapel]

That page needs to be removed and just point to the github README. I thought we did that a while ago. @scsides Was there discussion/work on that page when these changes happened?

[scsides]

Agreed, that ISIS doc web page should be removed. No discussion, because I thought it had been removed with the new install page was created on github.

[oleg-alexandrov]

But your github readme also says ISIS3DATA, while the code says ISISDATA.

[scsides]

That needs to be updated. ISIS3DATA is no longer used at all. If it exists somewhere, other than the preference file, it needs to be changed.