Do not validate for http:// URLs in elements and attributes that are …

[xoriath]

…definitely part of a different namespace

Since the XML parser does not seem to have much support at all for namespaces, this fix is very ... raw in that it only looks for a namespace delimiter : and if we find that in an element, mark it as ok. If we find it in an attribute, do not check it.

This fixes #2093

[thorstendb-ARM]

Changes seem to fix the issue for Atmel/Microchip, but not sure if this approach is a too generic.

I'd suggest the following:

/**
 * @brief visitor callback for files found in PDSC description
 * @param item RteItem file item
 * @return VISIT_RESULT
*/
VISIT_RESULT CheckFilesVisitor::Visit(RteItem* item)
{
  const string& tag = item->GetTag();
  if(tag == "environment") {
    return VISIT_RESULT::SKIP_CHILDREN;
  }

  m_checkFiles.CheckFile(item);
  m_checkFiles.CheckUrls(item);
  m_checkFiles.CheckDeprecated(item);
  m_checkFiles.CheckDescription(item);
  m_checkFiles.GatherIncPathVsAttrConfig(item);
  m_checkFiles.CheckCSolutionEntries(item);


  return VISIT_RESULT::CONTINUE_VISIT;
}

Please also change the test file to be more generic (no references to an existing vendor).

[thorstendb-ARM]

(comment merged, but cannot delete)

Specific environments should be ignored

[github-actions]

Test Results

  6 files   12 suites   11s ⏱️
 48 tests  48 ✅ 0 💤 0 ❌
144 runs  144 ✅ 0 💤 0 ❌

Results for commit e631eed.

♻️ This comment has been updated with latest results.

[xoriath]

Changes seem to fix the issue for Atmel/Microchip, but not sure about if this is a too generic approach.

I'd suggest the following:

/**
 * @brief visitor callback for files found in PDSC description
 * @param item RteItem file item
 * @return VISIT_RESULT
*/
VISIT_RESULT CheckFilesVisitor::Visit(RteItem* item)
{
  const string& tag = item->GetTag();
  if(tag == "environment") {
    return VISIT_RESULT::SKIP_CHILDREN;
  }

  m_checkFiles.CheckFile(item);
  m_checkFiles.CheckUrls(item);
  m_checkFiles.CheckDeprecated(item);
  m_checkFiles.CheckDescription(item);
  m_checkFiles.GatherIncPathVsAttrConfig(item);
  m_checkFiles.CheckCSolutionEntries(item);


  return VISIT_RESULT::CONTINUE_VISIT;
}

Please also change the test file to be more generic (no references to an existing vendor).

@thorstendb-ARM So this is not generic enough... A namespace can be declared at any level of the DOM, and thus there might be other url-like things...

Now, maybe the more correct thing is to just not parse anything namespace-like into the RTE model?

[thorstendb-ARM]

@thorstendb-ARM So this is not generic enough... A namespace can be declared at any level of the DOM, and thus there might be other url-like things...

The CMSIS Pack Description format does (currently) not use any namespaces. The extension point <environment> allows pack vendors to hook in to add data for other tools. The PackChk does verify some entries for environments "UV" and "DS5", but does not touch others. Thus, vendors are free to put whatever they like here. Most of the checks of PackChk should exclude anything below environment as it is not part of the CMSIS-Pack specification. This is what my code proposal does for this module.

Now, maybe the more correct thing is to just not parse anything namespace-like into the RTE model?

The RTE Model is written as a generic library, it reads everything that is in the pdsc file and presents it to a consumer.

[xoriath]

The thing that still bugs me is, the checking of xmlns: attributes which are allowed to be on any element which will declare a namespace is still being validated and forced to be https, which is really not what the original check intended to verify... But just ignoring most environments works for me specifically so good enough for me 😉

[edriouk]

Looks good for me, thanks you for contribution

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.32%. Comparing base (630fd22) to head (e631eed).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../packchk/test/integtests/src/PackChkIntegTests.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
- Coverage   65.37%   65.32%   -0.06%     
==========================================
  Files         182      182              
  Lines       37096    37099       +3     
  Branches    21561    21562       +1     
==========================================
- Hits        24252    24234      -18     
- Misses       8458     8480      +22     
+ Partials     4386     4385       -1     

Flag	Coverage Δ
packchk-cov	70.18% <75.00%> (-0.51%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
tools/packchk/src/CheckFiles.cpp	70.54% <100.00%> (-3.68%)	⬇️
.../packchk/test/integtests/src/PackChkIntegTests.cpp	71.22% <0.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.