WIP: opt-in to make target repo writable

[mathias-luedtke]

fixes #122 and probably #121

I really don't like making the src writable again, as the build should never mess with the src folder.
However, it was writable before and this fix improves the user experience.

[mathias-luedtke]

For now I would prefer to merge #120 instead.
If the TARGET_REPO_PATH is read-only, the run_ci is non-intrusive.

[130s]

(Hmm this title (ﾟдﾟ))

I saw #122 where existing users might prefer the current behavior, in which they can write into source space.
Given some comments from OSRF, however, it's safer to not use source space during the build.

There is an ideal in catkin that the source space should be treated as read only.

While right thing IMO to do may be to force not to use source space (and correct the sample code on travis-ci.org), merging this PR is okay temporarilly for me, to maintain the same behavior that was previously functioning (like #122 case).

[mathias-luedtke]

For now I would suggest to not merge it, but keeping it open for voting :)

[130s]

For now I would suggest to not merge it, but keeping it open for voting :)

Ok.

I just slightly changed the title, to reflect the context of this change better (and also to remove possible unintended political implication ;) ).

[Jmeyer1292]

I understand why you want to enforce "best practices", but the consistency of the user experience for an application like this is the most important feature. I don't need you to save me from myself. Deprecate the behavior, print a notice, and make the change in some major version release. Unless I force git to checkout a specific commit of this library, you can break my tests at will. In my humble opinion, +1.

[mathias-luedtke]

but the consistency of the user experience for an application like this is the most important feature.

We are refactoring a lot (#98) and this leads to regression bugs as well.
However, the goal is not only to provide a good user experience, but industrial-grade testing.
(Unfortunately we don't have unit tests for every feature yet (#112).)

Side effects like patching within the source are a great opportunity to shoot yourself in the foot, so this should be prevented by default.
I really understand that this is annoying (I had to patch some of my packages as well because of this).
If you want to stick to a certain version of industrial_ci, you can use the version tags.
I guess we should send out release notifications more often in the future.

Deprecate the behavior, print a notice, and make the change in some major version release.

It is hard to provide releases if everyone is cloning the source HEAD..
The all-in-docker feature was very disruptive internally, but provides the basis for new feature like gitlab support and local testing.
#120 should be the last big change of internal workflow, it removes all working-directory-related side effects. This may break all scripts that relied on these (undocumented) side effects :-/
Please keep on reporting any issue that might arise. This just helps us to avoid these pitfalls in the future :)

[mathias-luedtke]

I have refactored this PR to be an opt-in flag instead.

[mathias-luedtke]

I have refactored this once again.
By copying the source, the code can be writable, but it does not break run_ci, because the writes don't affect the original files.

[asmodehn]

While I am of the opinion that "good tools prevent unwary users to shoot themselves in the foot", I am currently relying on pip to install python packages as editable (to get a clean python installation instead of a ROS __init__.py hack). And pip creates a package.egg-info in the project source (https://pip.pypa.io/en/stable/reference/pip_install/#editable-installs) , which is apparently needed while running the software from devel space.
So I am conflicted because, from a CMake/C++ background I consider writing in source a bad practice, but from a python point of view, source and build are the same thing, what matters is your import hierarchy (how sys.path is dynamically computed from PYTHONPATH, and .pth files on your environment, plus other trickeries).
So I would 👍 this, which is the only way I see to get proper python integration...

[mathias-luedtke]

Shouldn't pip/venv take care of adding the paths to PYTHONPATH?
From the docs for --src: Directory to check out editable projects into. The default in a virtualenv is "<venv path>/src". Why do you need to overwrite the default?

I am really struggling to open this door, as it would allow users to just set this option instead of fixing the underlying issues

[asmodehn]

pip/venv dont do anything to the PYTHONPATH. quick demo :

alexv@AlexV-Linux:~$ mkvirtualenv ppath_test
New python executable in /home/alexv/.virtualenvs/ppath_test/bin/python
Installing setuptools, pip, wheel...done.
(ppath_test) alexv@AlexV-Linux:~$ echo $PYTHONPATH

(ppath_test) alexv@AlexV-Linux:~$ 

Even if it seem harsh I have to state this (given all the bad advices I see around the web) : Setting PYTHONPATH is NOT a proper way to set the python environment. It is at best a hack and at worst a demonstration that one does not understand how python import works.
How do python and virtualenv work? : http://pyvideo.org/pycon-us-2011/pycon-2011--reverse-engineering-ian-bicking--39-s.html

Incidentally ROS uses the PYTHONPATH in a way that makes it incompatible with existing de-facto python standard like virtual environments. Hence my long struggle with catkin_pip, which cannot use virtualenv. It ultimately boils down to "Workspaces stack up (and confuse everyone), virtualenvs just dont." plus a bunch of other incompatible trickeries.

The --src is one thing, quite debatable (see pyros-dev/catkin_pip#142)

However the other one, I am not aware of anything I can do to work around it at this time :

https://pip.pypa.io/en/stable/reference/pip_install/#editable-installs especially says :

For local projects, the "SomeProject.egg-info" directory is created relative to the project path.

[130s]

Not sure if this PR is review-merge-ready, but if it is then +1 for backward compatibility, no matter how long it's been since the disruptive change.

I'm just not sure if readers of this feature get what it means, because I'm afraid people may call "target source" in their own way. Personally in ROS world "workspace" is the most specific but I don't know.

[130s]

target source could better describe what actually points to. target source in your workspace?

[mathias-luedtke]

the source in TARGET_REPO_PATH?
(all other src folders are writebale anyway..)

[asmodehn]

Maybe catkin tools or CMake already has a variable name for this and we can reuse it ? like PROJECT_SOURCE_DIR or so? https://cmake.org/Wiki/CMake_Useful_Variables

[130s]

+1 for PROJECT_SOURCE_DIR. That sounds more common.

Meanwhile TARGET_REPO_PATH is probably an equivalent in industrial_ci. Either way, it may not be something that is exposed to end users is it as an API, except in document like this thread? So we could keep TARGET_REPO_PATH in the source code while using PROJECT_SOURCE_DIR in document, as an idea.

[mathias-luedtke]

First of all I still not recommend to merge it.
Second, WRITABLE_SOURCE is not a proper name for it.

Something like WRITES_INTO_TARGET_REPO ?
Or WRITABLE_TARGET_REPO.

[JWhitleyWork]

I know this has been a long-standing issue and no one has commented on this in some time but I wanted to mention a situation that I found where this is a problem and I don't see any other solution. I happened across the mavros repo recently and saw that the author had created a commit to start using industrial_ci. However, he quickly gave up because a read-only file system in the src folder caused the catkin CMake macro em_expand to be unable to write its output file within the source directory of the project. As the em_expand() function still exists in the current version of catkin, it seems like this is a perfect use-case for allowing writable src subdirectories.

[mathias-luedtke]

As the em_expand() function still exists in the current version of catkin, it seems like this is a perfect use-case for allowing writable src subdirectories.

@JWhitleyAStuff: Thanks for pointing this out!
Auto-generated files belong into the binary directory and must be "Installed" to devel and install, if needed by other packages. As an alternative they could be generated to install direcly, this depends on the nature of the file.
em_expand is used this way throughout the catkin source code..
Genererating files into the source folder is error prone, because these files could be modified (instead of the template) and after the next build the changes would be lost.

[130s]

@ipa-mdl I prefixed the title with "WIP". I've liked that practice to communicate better.

Have you changed mind about this PR to move forward?

[markusgft]

Is this pullrequest likely to be merged at some point? I would be very interested in having a writable src folder in travis CI.
My main motivation is that I want to generate part of the source files during the build. For simple examples, consdier this post here .

[mathias-luedtke]

Is this pullrequest likely to be merged at some point?

I don't think so.

I would be very interested in having a writable src folder in travis CI.

Please try #361. Despite the branch name, it still uses catkin_tools for ROS1.
It only supports the install space, to it was okay to make the src writable.
It will become the default in the next months.

install:
  - git clone --quiet --depth 1 https://github.com/ipa-mdl/industrial_ci.git -b colcon-rebased .industrial_ci

[markusgft]

Thanks for the quick reply and the pointer!

[mathias-luedtke]

I better close this now, #361 is the way to go.