[util/vendor_hw.py] add only_subdir support #594

tjshep · 2019-10-23T21:05:28Z

We would like to use the vendor_hw.py script to pull sw/host/vendor/mpsse/*.[ch] out of where they came from which is subdirectory in chromiumos/platform2 repo.

sw/host/vendor/mpsse.vendor.hjson would contain:

{
  name: "mpsse",
  target_dir: "mpsse",

  upstream: {
    url: "https://chromium.googlesource.com/chromiumos/platform2/",
    rev: "master",
    only_subdir: "trunks/ftdi",
  },
}

imphil · 2019-10-23T21:39:57Z

We already have exclude_from_upstream in the vendor description file (see https://bubble.servers.lowrisc.org/doc/ug/vendor_hw.html, section "How to exclude some files from the upstream repository").

Can you

Align the implementations, e.g. by creating an include_from_upstream key?
Define which of the two should have precedence.

tjshep · 2019-10-24T13:57:02Z

We already have exclude_from_upstream
[...]
Can you

Align the implementations, e.g. by creating an include_from_upstream key?

Define which of the two should have precedence.

That is exactly the road I started down (I started adding an include_only_from_upstream following the design of exclude_from_upstream), but also wanted to trim the prefix of the path name, so I started adding the ability to do that too, then realized that the path prefix trimming would imply include_only_from_upstream for everything down that path prefix, and was working out how to do that, then realized that with that working I wouldn't need the include_only_from_upstream anymore.
(Clearly, if we had both, there's no precedence problem, in order to be copied it should get past both filters... first do the include_only_from_upstream and then take what's left excluding anything caught by the exclude_from_upstream filter.)

But in any case, I don't need all that for the mpsse stuff. This much simpler only_subdir will do.

(And note that the upstream repo we want to fetch these four files from have over 8 thousand files we don't want, and those 8000+ files are scattered among over a hundred top level directories in that repo. So trying to use exclude_from_upstream would be an ongoing maintenance headache. And we'd still want to trim the path prefix.)

An include_only_from_upstream might be useful in some other case, but wouldn't help us here, and we would still want the path prefix trimming, and the only_subdir functionality I've implemented here seems to be the simplest way there.

I do welcome a better idea, but I don't think an include{_only,}_from_upstream sort of thing is what we need here (for mpsse), at least not as I imagined it and started implementing it yesterday.

imphil

Thanks for the explanation @tjshep, sounds reasonable.

The implementation LGTM with a couple nits, but please

Update the documentation in doc/rm/vendor_hw_tool.md to inform users about this new feature.
Update the commit message to explain a bit of your motivation (e.g. what you wrote in a previous comment here).

I'd encourage you to include a short "why did I do this change" paragraph in the commit message and pull request description right from the start, as it makes reviewing much easier, and helps in the future to understand why changes were done.

imphil · 2019-10-24T14:05:32Z

util/vendor_hw.py

+        if 'only_subdir' in desc['upstream']:
+            upstream_only_subdir = desc['upstream']['only_subdir']
+            clone_subdir = clone_dir / upstream_only_subdir
+            if not os.path.isdir(clone_subdir):


clone_subdir.is_dir() (clone_subdir is a Path object)

imphil

LGTM with the nits in the documentation addressed. Thanks!

imphil · 2019-10-24T22:04:49Z

doc/rm/vendor_hw_tool.md

+
+In the example vendor description file below, the mpsse directory is populated from
+the chromiumos platform2 repository, extracting just the few files in the
+trunks/ftdi subdirectory.


Please write one sentence per line in Markdown (https://bubble.servers.lowrisc.org/doc/rm/markdown_usage_style.html).

imphil · 2019-10-24T22:05:15Z

doc/rm/vendor_hw_tool.md

@@ -77,6 +77,32 @@ Optional parts can be removed if they are not used.
 }
 ```

+If only the contents of a single subdirectory (including its children) of an
+upstream repository is to be copied in, the optional `only_subdir` key of can be


imphil · 2019-10-24T22:05:34Z

doc/rm/vendor_hw_tool.md

@@ -77,6 +77,32 @@ Optional parts can be removed if they are not used.
 }
 ```

+If only the contents of a single subdirectory (including its children) of an
+upstream repository is to be copied in, the optional `only_subdir` key of can be
+used in the upstream section to specify the subdirectory to be copied in.  The


upstream -> upstream

…tion. With an 'only_subdir' key in an upstream section, a single subtree of an upstream repository can be specified for copying in. This will enable use of this tool to extract a small piece of a large upstream repository without having to keep the path of directories down to that piece and without having to maintain a large list of excludes.

This closes lowRISC#594. Signed-off-by: Andreas Kurth <adk@lowrisc.org>

tjshep requested review from gkelly, imphil and moidx October 23, 2019 21:05

tjshep requested a review from asb as a code owner October 23, 2019 21:05

imphil reviewed Oct 24, 2019

View reviewed changes

imphil mentioned this pull request Oct 24, 2019

vendor_hw script: Add include_from_upstream directive #64

Closed

tjshep force-pushed the vendor_hw_subdir branch from 672c86c to 13e58c6 Compare October 24, 2019 21:06

tjshep requested a review from sjgitty as a code owner October 24, 2019 21:06

tjshep requested a review from imphil October 24, 2019 21:14

imphil approved these changes Oct 24, 2019

View reviewed changes

tjshep force-pushed the vendor_hw_subdir branch from 13e58c6 to 4b3b8fe Compare October 25, 2019 14:54

tjshep force-pushed the vendor_hw_subdir branch from 4b3b8fe to d18ec40 Compare October 25, 2019 17:53

tjshep merged commit 24b7203 into lowRISC:master Oct 25, 2019

tjshep deleted the vendor_hw_subdir branch October 25, 2019 19:45

msfschaffner pushed a commit to msfschaffner/opentitan that referenced this pull request Nov 13, 2023

[top_darjeeling/dv] Cover testpoint chip_sw_soc_proxy_external_irqs

5ada659

This closes lowRISC#594. Signed-off-by: Andreas Kurth <adk@lowrisc.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[util/vendor_hw.py] add only_subdir support #594

[util/vendor_hw.py] add only_subdir support #594

tjshep commented Oct 23, 2019

imphil commented Oct 23, 2019

tjshep commented Oct 24, 2019

imphil left a comment

imphil Oct 24, 2019

imphil left a comment

imphil Oct 24, 2019

imphil Oct 24, 2019

imphil Oct 24, 2019

[util/vendor_hw.py] add only_subdir support #594

[util/vendor_hw.py] add only_subdir support #594

Conversation

tjshep commented Oct 23, 2019

imphil commented Oct 23, 2019

tjshep commented Oct 24, 2019

imphil left a comment

Choose a reason for hiding this comment

imphil Oct 24, 2019

Choose a reason for hiding this comment

imphil left a comment

Choose a reason for hiding this comment

imphil Oct 24, 2019

Choose a reason for hiding this comment

imphil Oct 24, 2019

Choose a reason for hiding this comment

imphil Oct 24, 2019

Choose a reason for hiding this comment