Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add verb for tracking and staging source changes in workspace #428

Closed
wants to merge 14 commits into from

Conversation

@dirk-thomas
Copy link
Member

Beside the question regarding the general approach mentioned in colcon/colcon-package-selection#44 (comment) I don't see a reason why this verb should be part of colcon-core. Is there any reason why this can't be contributed by a separate extension / Python package?

@ruffsl
Copy link
Member Author

ruffsl commented Apr 14, 2021

Is there any reason why this can't be contributed by a separate extension / Python package?

I don't think there should be anything preventing that. This was just a quick proof of concept, and there isn't yet a separate repo to host a draft of this kind of verb and task extension. I was just going with what was stated earlier in the discussion of this feature:

Let's figure that out on demand though - moving a package into a separate repo is straight forward anytime.
colcon/colcon-package-selection#25 (comment)

Is there a separate extension in the colcon ecosystem that I could use as a reference that implements both a verb and a task?

@dirk-thomas
Copy link
Member

Is there a separate extension in the colcon ecosystem that I could use as a reference that implements both a verb and a task?

Based on the context of the question I am not sure if that even makes sense here. Why would you need different tasks to implement stage for different package types? What custom logic are you expecting?


To answer the question independent of the context:

Maybe colcon-bundle? Most extensions really focus on one aspect only - a new verb or a task to an existing extension - and therefore only contain one of the two, e.g.:

  • Verb extensions: colcon-devtools, colcon-metadata, colcon-mixin, colcon-package-information, colcon-test-result
  • Task extensions: colcon-cmake, colcon-ros

But both should be orthogonal and you should be able to just put them together in the same package.

@ruffsl
Copy link
Member Author

ruffsl commented Apr 14, 2021

Why would you need different tasks to implement stage for different package types? What custom logic are you expecting?

Like I said, I'm not sure if a task is being used as intended, I just used it as a hack so I can feed jobs an extension that can hashdir:

extension = get_task_extension('colcon_core.task.stage', 'default')

job = Job(
identifier=pkg.name,
dependencies=set(recursive_dependencies.keys()),
task=extension, task_context=task_context)

Thus the hardcoded default package type nonsense, as my task doesn't care about the package type the verb encounters.

@ruffsl
Copy link
Member Author

ruffsl commented Apr 14, 2021

But both should be orthogonal and you should be able to just put them together in the same package.

Could you create a new repo on the colcon org so I have something to fork and target PRs to?

I am not convinced "audit" is the right term for this functionality. Nothing is being reviewed here. It is more like watching for changes in the file system. I don't have a good proposal atm though.

You didn't seem convinced of the verb audit, and I'd like to avoid hash or checksum, as we could have the tool just as well check git for diff changes rather than always using hashdir or deephash. Would package name colcon-stage work?

@dirk-thomas
Copy link
Member

Thus the hardcoded default package type nonsense, as my task doesn't care about the package type the verb encounters.

Got it. This concept makes sense for the build and test verb but probably not for your new verb. The Job instances your verb creates only need a task which provides some API (I think a set_context method and being callable). You can manually create an instance which satisfies that without needing to go through get_task_extension.

Could you create a new repo on the colcon org so I have something to fork and target PRs to?

I suggest you create a repo under your username for prototyping and development. That allows you to iterate without me having to be in the loop (especially since I am quite out of the loop of colcon nowadays).

We can transfer the repository to the colcon org later. (You could even release from your private repo - no technical reason not to - mostly for visibility and future access, see https://colcon.readthedocs.io/en/released/developer/contribution.html#new-packages-extensions)

You didn't seem convinced of the verb audit, ... Would package name colcon-stage work?

Naming is obviously hard 😉 Imo both - audit and stage - have a specific meaning in the context they are commonly used in Neither seems to apply to what this verb is doing. Nothing is really inspected / examined, unstaging isn't possible, also there is no step after staging.

For transparency to the user I think a verb to not only show which which packages have changed since the last manual invocation but also why.

I don't have a specific proposal. Ultimately you should choose what you want - I shouldn't be the BDFL. Maybe reach out to a bigger audience to get ideas / feedback (or after prototyping with some name before releasing)? With the above comment about additional verbs in mind maybe two-level (like metadata or mixin) would make sense here? Just some random ideas:

  • reference reset (rehashing all), reference diff (show what has changed in each package), reference info (maybe information about when the last reset occurred, or if some package has never been hashed at all)
  • baseline (instead of reference with similar subverbs)

I'd like to avoid hash or checksum, as we could have the tool just as well check git for diff changes rather than always using hashdir or deephash.

I agree with the general idea to avoid terms derived from implementation details which could very well change in the future. That being said I don't think using git diff is a viable option since the directories might use different vcs or perhaps no vcs at all (e.g. when using tarballs from releases downloaded with rosinstall_generator and vcstool).

@dirk-thomas dirk-thomas added the enhancement New feature or request label Apr 15, 2021
ruffsl added a commit to ruffsl/colcon-cache that referenced this pull request Apr 18, 2021
ruffsl added a commit to ruffsl/colcon-cache that referenced this pull request Apr 18, 2021
ruffsl added a commit to ruffsl/colcon-cache that referenced this pull request Apr 19, 2021
* Stage subverb template from colcon metadata

* Update subverb template

* Stage ListMetadataSubverb as template

* Update subverb template

* Create capture task submodule

* Stage PythonStageTask as template
from colcon/colcon-core#428

* Refactor snapshot capture task

* Stage StageVerb as template
from colcon/colcon-core#428

* Stage refactor

* Rename file

* Stage working task

* Stage template from dirhash

* Stage WIP on git task

* Change to markdown

* Fix linter errors
@ruffsl
Copy link
Member Author

ruffsl commented May 24, 2021

Closing in favor of ruffsl/colcon-cache#12 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants