Skip to content

fix(build): use container directories in executable_search_paths when building in container#1030

Merged
sanathkr merged 6 commits intoaws:developfrom
sanathkr:gradlefix
Feb 28, 2019
Merged

fix(build): use container directories in executable_search_paths when building in container#1030
sanathkr merged 6 commits intoaws:developfrom
sanathkr:gradlefix

Conversation

@sanathkr
Copy link
Contributor

@sanathkr sanathkr commented Feb 28, 2019

Fixes #1029

Issue #, if available:

Description of changes:

Checklist:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@sriram-mv sriram-mv Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we add kwarg mapping.

Clarification:

Add kwarg mapping to the method call _convert_to_container_dirs

_convert_to_container_dirs(host_paths_to_convert=executable_search_paths, host_to_container_path_mapping={source_dir:containers_dir["source_dir"],manifest_dir: containers_dir["manifest_dir"]})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is under the assumption that, the base path we are looking for, is already mounted within the container correct?

eg:
/a/b is already mounted within container, and we are just passing additional /a/b/c as additional path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, result is a list. I am just adding elements to the list without altering the container paths themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class -> method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok this is a passthrough, but searching this path within the container will not turn up anything right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect 🔥

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, if the path that is supplied to be added as executable_search_path within the container is already present in the mapping of paths to be mounted, we add the path to be mounted within the container to list of executable search paths.

Also this is very opinionated to adding just source_dir or manifest_dir within the container to search paths, and not any other sub paths.

eg: instead of just /tmp/source_dir and /tmp/manifest_dir , we dont yet support /tmp/source_dir/myexecutable_versions/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we don't support that. This is a good call out. We should be okay for the gradlew case now, but we definitely need to implement a generic path-translation logic that handles lot of these cases

@sriram-mv sriram-mv self-assigned this Feb 28, 2019
Copy link
Contributor

@sriram-mv sriram-mv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for engaging through my long list of questions 🥇

@sanathkr sanathkr merged commit d20cb67 into aws:develop Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants