-
Notifications
You must be signed in to change notification settings - Fork 24
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
Updated Makefile and docs for fedsd utility #262
Conversation
BTW, |
It works this way, but seems more like a hack... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem with the fix to the Makefile is that now a file in /usr/local/bin is a symbolic link into a user's clone of the repo. This seems problematic to me. Would it work instead to copy launch-fedsd.sh into /usr/local/bin (or wherever is specified with INSTALL_PREFIX)?
Also, for the other changes that try to prepend a path to trace_to_csv using an INSTALL_PREFIX argument to fedsd, this seems awkward. If trace_to_csv is in the user's PATH, it seems like it would not be necessary. Can we just check to see whether trace_to_csv is in the user's path and issue a friendly error message if not?
I am sorry about this. I didnt test the fedsd utility. I think I agree with Edward, it is not unreasonable to demand that for |
What is, btw, the reason for having this launch shell |
The Zephyr test failure is fixed by updating LF ref here: #263 |
No need for the launch script, IMO. Let make install copy the Python script and be done with it? Make sure it allows for specifying an installation prefix... |
Can a python program be made executable? I was unaware of that... How does the OS know which python to invoke? |
|
Just pushed a fix, where A detail worth mentioning in this update is that one does not need saying Regarding |
What configuration effort? On Windows, a bash script must be executed in WSL, and in WSL running a script with a |
Yes yes, you're right! Same for Cygwin. |
Since the introduction of WSL, there is a whole lot less to worry about. We can just tell Windows users to use WSL... |
To build confidence in multi-platform support, just add some tests in CI and let them run them on all platforms. I don't believe there are currently any tests, so please add those. |
Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: Marten Lohstroh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo suggestions, this looks good, but there are still no tests, so we don't have any assurance that this works and will keep working...
Since it appears that what's in |
When installing the tracing executables to
/usr/local/bin
,fedsd
will not work, because it is operating on a dangling symlink.This PR provides a fix.