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

Fix gdb scripts #13658

Merged
merged 6 commits into from
Mar 16, 2020
Merged

Fix gdb scripts #13658

merged 6 commits into from
Mar 16, 2020

Conversation

jyapayne
Copy link
Contributor

@krux02 I think I've addressed your issues, except for the function call. Let me know if it works for you and if there are any other changes you want.

bin/nim-gdb.bash Outdated
(echo "readlink not in PATH. Please install coreutils from homebrew."; exit 1)) || \
(echo "readlink not in PATH."; exit 1)

nreadlink () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is needed because a variable can't be called in the below:

NIM_SYSROOT=$(dirname $(dirname $(nreadlink -e $(which nim))))

If nreadlink is instead a variable, the command will fail to run.

bin/nim-gdb.bash Outdated Show resolved Hide resolved
@krux02
Copy link
Contributor

krux02 commented Mar 16, 2020

I am not much a fan of the file name change. The file is in my path and therefore the name truly matters. And the only reason the change happend was because of a bug in VSCode, am I right? Can't you do it the other way around, keep the original name without ending, but you put an additional symbolic link with the name nim-gdb.bash into the project? I would be much happier with that.

@jyapayne
Copy link
Contributor Author

@krux02 Sure, that works too. Done.

@Araq
Copy link
Member

Araq commented Mar 16, 2020

Once again the shell scripts are turned into edit wars. Lesson learned, no shell script will ever be accepted again.

@Araq Araq merged commit fd35838 into nim-lang:devel Mar 16, 2020
@krux02
Copy link
Contributor

krux02 commented Mar 16, 2020

@Araq As a short recap. You merge a PR for a file where you know very well that I am responsible for without any review process and without giving me time or opportunity to comment on it. Then when problems bubble up you use the so called "edit wars" as pretext to ban shell scripts all together. Ok then, go for it.

@Araq
Copy link
Member

Araq commented Mar 16, 2020

Without my merge the facts remain identical: Shell scripts never suit everybody and cause lots of maintainance overhead.

@krux02
Copy link
Contributor

krux02 commented Mar 16, 2020

Well then, the same can be said about nim source code. Do you want to ban nim code as well?

@jyapayne jyapayne deleted the fix_debug_script branch March 16, 2020 12:43
@Araq
Copy link
Member

Araq commented Mar 17, 2020

The same cannot be said about Nim code as Nim code is much more portable than Shell code. But yeah, we also actually we try to have fewer lines of code written in Nim, see how @timotheecour and others must fight to get something into the stdlib or the compiler.

@timotheecour
Copy link
Member

timotheecour commented Mar 17, 2020

@krux02 Am I missing something or everything in bin/nim-gdb and bin/nim-gdb.bat could be turned into tools/nimgdb.nim that would achieve the exact same but cross platform / more portable? (say with osproc.startProcess)
IMO the only shell script need in Nim repo is what's needed to bootstrap (build_all.sh/build_all.bat); once nim is bootstrapped, everything should use nim, and would result in simpler & smaller code.
Including things like

tests/dll/test_nimhcr_integration.sh (not portable! linux but not OSX nor windows)

shell scripts are bad at code reuse, portability, etc

@krux02
Copy link
Contributor

krux02 commented Mar 17, 2020

First of all, it should be said, I am not a fan of nim-gdb to exist in the first place. I tried my best to make running gdb just work. But the mechanisms in gdb to automatically load pretty printers don't work for programming languages, they only work for libraries. So I did the best I could do. And I tried to keep it as minimal as possible. But one requirement for nim-gdb was that it would actually become a real gdb process, not some shell process or nim process that just runs gdb. This ensures that signals sent to nim-gdb actually reach the gdb process and therefore the signal handler of gdb. So the exec in the last is very important.

Am I missing something or everything in bin/nim-gdb and bin/nim-gdb.bat could be turned into tools/nimgdb.nim that would achieve the exact same but cross platform / more portable? (say with osproc.startProcess)

Yes you are missing something. startProcess or execProcess both start new subprocesses, wich is not wanted (like I just explained above). exec is what is required. This does exist in Nim here, but this is platform specific again. So eventually I went with shell scripts in favor of Nim.

jyapayne added a commit to jyapayne/Nim that referenced this pull request Mar 20, 2020
Araq pushed a commit that referenced this pull request Mar 20, 2020
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.

4 participants