Fix #4156 - Support nested subfolders in approved Measure directories #4417

jmarrec · 2021-08-25T13:42:19Z

Pull request overview

Fix #4156 - Support nested subfolders in approved Measure directories

(green) is what is now supported in addition to what was there. ! (orange) is what is still disallowed

 .
 ├── ./docs
+│   ├── ./docs/docs.rb
 │   ├── ./docs/.gitkeep
 │   └── ./docs/subfolder
+│       └── ./docs/subfolder/subfolder_file.txt
 ├── ./.git
!│   └── ./.git/index
 ├── ./.hidden_folder
!│   └── ./.hidden_folder/file.txt
 ├── ./LICENSE.md
 ├── ./measure.rb
 ├── ./measure.xml
+├── ./README.md
 ├── ./README.md.erb
 ├── ./resources
+│   ├── ./resources/resources.rb
 │   └── ./resources/subfolder
+│       └── ./resources/subfolder/subfolder_file.txt
!├── ./root_file.txt
 ├── ./subfolder
!│   └── ./subfolder/subfolder.txt
 └── ./tests
     ├── ./tests/example_model.osm
     ├── ./tests/output
!    │   └── ./tests/output/output.txt
     ├── ./tests/subfolder
+    │   └── ./tests/subfolder/subfolder_file.txt
     ├── ./tests/test_recursive_measure_test.rb
+    └── ./tests/tests.rb

TODO

TODO: do we keep ignoring the docs/ directory when we clone the measure?
TODO: should BCLMeasure(const openstudio::path&) call checkForUpdatesFiles? Should BCLMeasure::clone(const openstudio::path&) call checkForUpdatesFiles() first so we have a clean BCLXML to use for copying the files?

Pull Request Author

BCL API Changes / Additions
BCL API methods are tested (in src/utilities/test)
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

… conan

…ecause currently it does path.filename() so it's write "docs/subfolder/a.txt" as "a.txt" with usageType = doc, not subfolder/a.txt This requires breaking API, not sure where I do it like this in the Ctor to BCLFileReference or just in BCLFileReference::writeValues

jmarrec · 2021-08-25T13:45:15Z

src/utilities/bcl/BCLFileReference.cpp

+BCLFileReference::BCLFileReference(const openstudio::path& measureRootDir, const openstudio::path& relativePath, const bool setMembers)
+  : m_measureRootDir(openstudio::filesystem::system_complete(measureRootDir)),
+    m_path(openstudio::filesystem::system_complete(measureRootDir / relativePath)) {


API break. I have to somehow track the measureRootDir so I can change filename() below.

This was discussed on Slack, and I agree with @tijcolem that we can probably be a little more tolerant of occasional API changes in BCL, compared to the Model API. I'd recommend merge.

jmarrec · 2021-08-25T13:45:29Z

src/utilities/bcl/BCLFileReference.cpp

+  std::string usageType = this->usageType();
+  openstudio::path baseDir = m_measureRootDir;
+  if (usageType == "doc") {
+    baseDir /= "docs";
+  } else if (usageType == "resource") {
+    baseDir /= "resources";
+  } else if (usageType == "test") {
+    baseDir /= "tests";
+  }
+
+  return toString(openstudio::filesystem::relative(m_path, baseDir));


Actual required change for API break above

jmarrec · 2021-08-25T13:46:06Z

src/utilities/bcl/BCLMeasure.cpp

+static constexpr std::array<std::pair<std::string_view, std::string_view>, 4> rootToUsageTypeMap{{
+  // fileName, usageType
+  {"measure.rb", "script"},
+  {"LICENSE.md", "license"},
+  {"README.md", "readme"},
+  {"README.md.erb", "readmeerb"}
+  // ".gitkeep"      // assuming .gitkeep outside a subfolder makes zero sense...
+  // "measure.xml"   // not included in itself!
+}};


Approved rootFiles (and their automatic usageType) are declared in this single location

jmarrec · 2021-08-25T13:46:57Z

src/utilities/bcl/BCLMeasure.cpp

+static constexpr std::array<std::pair<std::string_view, std::string_view>, 3> approvedSubFolderToUsageMap{{
+  {"docs", "doc"},
+  {"tests", "test"},
+  {"resources", "resource"},
+}};
+
+static constexpr std::array<std::string_view, 1> ignoredSubFolders{"tests/output"};


Approved subfolders (and their automatic usageType) are declared in this single location, along with exclusions (one right now) to ingore the result of running openstudio measure --run-tests .

jmarrec · 2021-08-25T13:47:18Z

src/utilities/bcl/BCLMeasure.cpp

+// TODO: do we want to keep ignoring the docs/ directory?
+static constexpr std::array<std::string_view, 1> usageTypesIgnoredOnClone{"doc"};


TODO: do we keep ignoring the docs/ directory when we clone the measure?

jmarrec · 2021-08-25T13:57:10Z

src/utilities/bcl/BCLMeasure.cpp

+  // TODO: The logic here is weird. why are we creating then removing it?
+  // removeDirectory(newDir);


Suggested change

// TODO: The logic here is weird. why are we creating then removing it?

// removeDirectory(newDir);

jmarrec · 2021-08-25T13:59:55Z

src/utilities/bcl/BCLMeasure.cpp

+  // TODO: isn't it better to copy only what's inside measure.xml?
+  // The only caveat is what if the measure.xml is outdated? As a reminder, The BCL measure wouldn't have been loaded first, which updates the XML
+  // So the only potential issue is if the user loads the measure (in IRB/PRY for eg), then continues to make changes to the filesystem, then much
+  // later calls measure.clone. I don't think this is an issue

-  openstudio::path tests = toPath("tests");
-  if (exists(this->directory() / tests) && !this->copyDirectory(this->directory() / tests, newDir / tests)) {
-    return boost::none;
-  }
+  // Copy the measure.xml
+  openstudio::filesystem::copy_file_no_throw(m_directory / openstudio::path{"measure.xml"}, newDir / openstudio::path{"measure.xml"});
+
+  // Then copy whatever is referneced in the measure.xml
+  for (const BCLFileReference& file : this->files()) {

-  openstudio::path resources = toPath("resources");
-  if (exists(this->directory() / resources) && !this->copyDirectory(this->directory() / resources, newDir / resources)) {
-    return boost::none;
+    if (std::find_if(usageTypesIgnoredOnClone.cbegin(), usageTypesIgnoredOnClone.cend(),
+                     [&file](const auto& sv) { return file.usageType() == std::string(sv); })
+        == usageTypesIgnoredOnClone.cend()) {
+
+      // BCLFileReference::path() is absolute
+      openstudio::path oriPath = file.path();
+      openstudio::path relativePath = openstudio::filesystem::relative(oriPath, m_directory);
+      openstudio::path destination = newDir / relativePath;
+      // Create parent directories in destination if need be
+      openstudio::filesystem::create_directories(destination.parent_path());
+      openstudio::filesystem::copy_file_no_throw(oriPath, destination);
+    }


This is a change. Instead of relying on the files on disk, we should just rely on the fact that we've got a clean BCLXML... My comment is wrong though, checkForUpdatesFiles isn't called... perhaps we should just do that?

TODO: clone should call checkForUpdatesFiles() first?

jmarrec · 2021-08-25T14:00:37Z

src/utilities/core/Path.cpp

+path toPath(std::string_view s) {
+  return toPath(std::string(s));
+}


new toPath for std::string_view

jmarrec · 2021-08-25T14:01:02Z

src/utilities/bcl/test/BCLMeasure_GTest.cpp

+TEST_F(BCLFixture, isApprovedFile) {
+
+  EXPECT_TRUE(BCLMeasure::isIgnoredFileName(".hello"));
+  EXPECT_TRUE(BCLMeasure::isIgnoredFileName(""));
+  EXPECT_TRUE(BCLMeasure::isIgnoredFileName("."));
+  EXPECT_TRUE(BCLMeasure::isIgnoredFileName(".."));
+  EXPECT_FALSE(BCLMeasure::isIgnoredFileName(".gitkeep"));
+  EXPECT_FALSE(BCLMeasure::isIgnoredFileName("hello"));
+
+  openstudio::path measureDir = "/usr/output/my_measure";
+
+  std::vector<std::pair<openstudio::path, bool>> tests{
+    {fs::path("measure.rb"), true},
+    {fs::path("somethingelse.rb"), false},  // Non approved root file
+    {fs::path("docs/hello.rb"), true},
+    {fs::path("docs/subfolder/hello.rb"), true},
+    {fs::path("tests/hello.rb"), true},
+    {fs::path("tests/subfolder/hello.rb"), true},
+    {fs::path("tests/output/hello.rb"), false},  // excluded subfolder
+    {fs::path("resources/hello.rb"), true},
+    {fs::path("resources/subfolder/hello.rb"), true},
+    {fs::path("resources/output/hello.rb"), true},
+
+    // Not an approved subfolder
+    {fs::path("non_approved_subfolder/hello.rb"), false},
+    {fs::path("non_approved_subfolder/subfolder/hello.rb"), false},
+    {fs::path("non_approved_subfolder/output/hello.rb"), false},
+  };
+
+  for (const auto& [relativeFilePath, expectedAllowed] : tests) {
+    openstudio::path absoluteFilePath = measureDir / relativeFilePath;
+    EXPECT_EQ(expectedAllowed, BCLMeasure::isApprovedFile(absoluteFilePath, measureDir));
+  }
+}


Test isApprovedFile

jmarrec · 2021-08-25T14:01:31Z

src/utilities/bcl/test/BCLMeasure_GTest.cpp

+  }
+}
+
+TEST_F(BCLFixture, 4156_TestRecursive) {


Plenty of more testing, it should be commented enough, so I won't bother adding review comments here. It's quite long too.

…e need to discuss this and keep only one branch

tijcolem · 2021-09-20T15:07:30Z

@jmarrec This looks good but I see one failing unit test

The following tests FAILED:
	651 - BCLFixture.BCLMeasure (Failed)

I built this on my local mac as well and confirm that it is failing. On thing that stands is the number of files in an equality test. 6 vs 5.

/Users/tcoleman/git/OpenStudio/src/utilities/bcl/test/BCLMeasure_GTest.cpp:91: Failure
652: Expected equality of these values:
652:   6u
652:     Which is: 6
652:   measure2->files().size()
652:     Which is: 5

Would you mind looking at this failure? Otherwise I think this good to go.

…sure_Folder

jmarrec · 2021-09-21T14:21:49Z

src/utilities/bcl/BCLMeasure.cpp

-    if (m_bclXML.hasFile(srcItemPath) || (xmlPath == openstudio::filesystem::system_complete(srcItemPath))) {
-      if (!openstudio::filesystem::copy_file_no_throw(srcItemPath, dstItemPath)) {
-        return false;
+  if (parentPath == measureDir) {


This is where the unit test is failing @tijcolem .

Once again I got bit by the fact that toPath("/path/to/dir/") != toPath("/path/to/dir") in boost::filesystem::path...

(Path handling is absolutely infuriating in C++, whether that's boost::filesystem or std::filesystem)

5eff870 should have fixed this.

@jmarrec Thanks for addressing this. That is supper annoying that you can't compare equality using == with either of those. It looks like your last commit cleared up Mac and Linux but of course Windows is being finicky and still failing on the same test. I tried to launch a full build to make sure, but yep, still failing.

https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-develop-nightly/detail/openstudio-develop-nightly/1241/pipeline/280

jmarrec · 2021-09-21T14:34:21Z

src/utilities/bcl/BCLMeasure.cpp

+  // TODO: isn't it better to copy only what's inside measure.xml?
+  constexpr bool weCareAboutOutdatedMeasureXML = true;
+  // The only caveat is what if the measure.xml is outdated with regards to <files>?
+  // Currently the BCLMeasure(const path&) ctor **does NOT** call checkUpdatesFiles()
+  //
+  // We could call it here by that would require a const_cast and it would increment the measure.xml versionId of the original measure,
+  // and update the list of files, which isn't a big deal if the user doesn't then save it too? Even if the user did it, wouldn't we be helping out anyways?
+
+  // If this is really not wanted, we can go back to scanning the files on disk, with a similar logic as in checkForUpdatesFiles:
+  // loop on all files on disk, and if isApproved is true, then copy them
+
+  // Copy the measure.xml
+  openstudio::filesystem::copy_file_no_throw(m_directory / openstudio::path{"measure.xml"}, newDir / openstudio::path{"measure.xml"});


TODO: we need to decide and keep one of these path @kbenne @tijcolem

…les in the xml. And separate into two tests for readability

…sure_Folder

ci-commercialbuildings · 2021-10-01T15:32:32Z

CI Results for 7b9161d:

Ubuntu 18.04:
- OpenStudio-3.3.0-alpha+7b9161d6d2-Ubuntu-18.04.deb
- OpenStudio-3.3.0-alpha+7b9161d6d2-Ubuntu-18.04.tar.gz
Ubuntu 20.04:
- OpenStudio-3.3.0-alpha+7d992c9bf9-Ubuntu-20.04.deb
- OpenStudio-3.3.0-alpha+7d992c9bf9-Ubuntu-20.04.tar.gz
Darwin:
- OpenStudio-3.3.0-alpha+7b9161d6d2-Darwin.dmg
- OpenStudio-3.3.0-alpha+7b9161d6d2-Darwin.tar.gz
Windows:
- OpenStudio-3.3.0-alpha+7d992c9bf9-Windows.exe
- OpenStudio-3.3.0-alpha+7d992c9bf9-Windows.tar.gz
Code Coverage:
- OpenStudio-7b9161d6d256c41c2c6df2773161adf827a8af83.gcov.html

jmarrec added 10 commits August 23, 2021 17:57

Add some notes, and use replace_all in boost now that we use 1.73 via…

a49bd12

… conan

Add a unit test for #4156

ccc8e4d

Temporary unit test for dealing with ignoredSubDirectory logic

7d3670f

Write full unit tests for BCLMeasure

0ff124a

modernize code

e6f3e1b

Make changes to BCLMeasure to support nested subdirectories

75384c6

Add a test for the case where a user might manually tweak the XML

dfc4140

Turn off BCL standardout logger

371a413

performance: reserve() before std::transform

a767ddd

jmarrec added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Aug 25, 2021

jmarrec requested review from axelstudios and tijcolem and removed request for axelstudios August 25, 2021 13:43

jmarrec self-assigned this Aug 25, 2021

jmarrec added component - Measure Manager component - Utilities Other Enhancement Request labels Aug 25, 2021

jmarrec commented Aug 25, 2021

View reviewed changes

jmarrec added the APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated label Aug 25, 2021

jmarrec added 2 commits August 25, 2021 16:46

Add BCLFileReference::relative_path( ) for convenience

f9ec30c

Add an alternative for BCLMeasure::clone + a test for outdated XML. W…

983b41a

…e need to discuss this and keep only one branch

Merge remote-tracking branch 'origin/develop' into 4156_Recursive_Mea…

9699693

…sure_Folder

jmarrec commented Sep 21, 2021

View reviewed changes

jmarrec added 2 commits September 21, 2021 16:35

Debug why BCLMeasure test is failing

cf57c91

Remove trailing separator to avoid being bit

5eff870

jmarrec force-pushed the 4156_Recursive_Measure_Folder branch from 1d35fa7 to 5eff870 Compare September 21, 2021 14:35

jmarrec added 2 commits September 29, 2021 12:37

Merge branch 'develop' into 4156_Recursive_Measure_Folder

35c19c7

Remove duplicated code in unit test

30c6d9b

shorowit mentioned this pull request Sep 29, 2021

Update to EnergyPlus v9.6/OpenStudio v3.3 NREL/OpenStudio-HPXML#839

Closed

10 tasks

jmarrec added 4 commits September 29, 2021 13:52

Made a mistake in usageType of the report.html.in

5ab91ed

Enhance unit tests to also check the usageType and relativePath of fi…

5234756

…les in the xml. And separate into two tests for readability

Merge remote-tracking branch 'origin/develop' into 4156_Recursive_Mea…

d85608c

…sure_Folder

Make cppcheck happy

7b9161d

tijcolem approved these changes Oct 5, 2021

View reviewed changes

tijcolem merged commit 81c7067 into develop Oct 5, 2021

tijcolem deleted the 4156_Recursive_Measure_Folder branch October 5, 2021 02:28

shorowit mentioned this pull request Dec 14, 2021

Reorganize resource files NREL/OpenStudio-HPXML#935

Merged

6 tasks

jmarrec mentioned this pull request Dec 16, 2022

Make sorting of files in measure.xml consistent #4748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4156 - Support nested subfolders in approved Measure directories #4417

Fix #4156 - Support nested subfolders in approved Measure directories #4417

jmarrec commented Aug 25, 2021 •

edited

Loading

jmarrec Aug 25, 2021

kbenne Sep 20, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

jmarrec Aug 25, 2021

tijcolem commented Sep 20, 2021

jmarrec Sep 21, 2021 •

edited

Loading

jmarrec Sep 21, 2021

jmarrec Sep 21, 2021

tijcolem Sep 22, 2021

jmarrec Sep 21, 2021

ci-commercialbuildings commented Oct 1, 2021 •

edited

Loading

		// TODO: do we want to keep ignoring the docs/ directory?
		static constexpr std::array<std::string_view, 1> usageTypesIgnoredOnClone{"doc"};

		// TODO: The logic here is weird. why are we creating then removing it?
		// removeDirectory(newDir);

Fix #4156 - Support nested subfolders in approved Measure directories #4417

Fix #4156 - Support nested subfolders in approved Measure directories #4417

Conversation

jmarrec commented Aug 25, 2021 • edited Loading

Pull request overview

TODO

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tijcolem commented Sep 20, 2021

jmarrec Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Oct 1, 2021 • edited Loading

jmarrec commented Aug 25, 2021 •

edited

Loading

jmarrec Sep 21, 2021 •

edited

Loading

ci-commercialbuildings commented Oct 1, 2021 •

edited

Loading