r-lib / pkgbuild

Find tools needed to build R packages
https://pkgbuild.r-lib.org
Other
65 stars 33 forks source link

Could `compile_dll()` have an option to pass on `--debug` to INSTALL? #135

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 2 years ago

I was attempting to get gdb to work with a package that compiles C++ code, and wasn't having much luck because I kept getting this from gdb when I did something like info args

no symbol table info available

It turns out that, on Windows, the linker gets passed -s to strip out the symbol tables unless the --debug flag is set at INSTALL time. https://cs.github.com/wch/r-source/blob/36e90ea062c4fad5343ae3a4ca122a3b266ad354/src/gnuwin32/fixed/etc/Makeconf#L10

The only reprex I can offer is to point you to https://github.com/DavisVaughan/vsexample

You should be able to reproduce what I'm seeing by:

At this point you can try and look around with info args but I get the "no symbol table" stuff from above.

I manually passed "--debug" to pkgbuild:::install_min() as an extra args value and that seemed to work. I was able to repeat the steps above and then the symbol info was available.

gaborcsardi commented 2 years ago

I forgot where this is documented, but on Windows you need to set the DEBUG env var to true and then R uses the right flags when installing packages. In my experience this works pretty well, and there is no need to set any flags anywhere manually.

  1. Install gdb
  1. Set the DEBUG=true env var, e.g. in a powershell Windows terminal:

    $env:DEBUG="true"
  2. In the same terminal go to your package, and recompile it. E.g. for me this is

    cd ~/works/processx
    git clean -fdx src
    R-devel.bat -q -e "pkgload::load_all()"

    You should see the -gdwarf-2 flag when compiling.

  3. Start R in gdb, and happily set breakpoints, etc.

    C:\rtools40\ucrt64\bin\gdb "C:\Program Files\R\R-devel\bin\x64\Rterm.exe"

    You can press CTRL+C to drop back to gdb from R.

DavisVaughan commented 2 years ago

Ah I think setting DEBUG=T is exactly what --debug ends up doing. i.e. if you follow --debug through INSTALL to SHLIB you get to this, which sets the corresponding env var when make is being run: https://cs.github.com/wch/r-source/blob/36e90ea062c4fad5343ae3a4ca122a3b266ad354/src/library/tools/R/install.R#L2700

I did see that R for Windows FAQ recommends --debug (CMD + F for --debug) https://cran.r-project.org/bin/windows/base/rw-FAQ.html

gaborcsardi commented 2 years ago

Yeah, so R CMD INSTALL does seem to pass --debug to R CMD SHLIB, even though this flag is not documented for INSTALL AFAICT, I didn't even know it existed. For SHLIB it is documented.

I think we could just document that people need to set DEBUG=true if they want to debug their package? And/or create a vignette with the process at https://github.com/r-lib/pkgbuild/issues/135#issuecomment-1076811172 that we could try to keep current?

DavisVaughan commented 2 years ago

It is documented for INSTALL

daviss-mbp-2:~ davis$ R CMD INSTALL --help
Usage: R CMD INSTALL [options] pkgs

Install the add-on packages specified by pkgs.  The elements of pkgs can
be relative or absolute paths to directories with the package
sources, or to gzipped package 'tar' archives.  The library tree
to install to can be specified via '--library'.  By default, packages are
installed in the library tree rooted at the first directory in
.libPaths() for an R session run in the current environment.

Options:
  -h, --help        print short help message and exit
  -v, --version     print INSTALL version info and exit
  -c, --clean       remove files created during installation
      --preclean    remove files created during a previous run
  -d, --debug       turn on debugging messages

^ HERE IT IS
<SNIP>

It even has some Windows specific docs that mention the debug DLL bit:

https://github.com/wch/r-source/blob/36e90ea062c4fad5343ae3a4ca122a3b266ad354/src/library/tools/R/install.R#L228-L229

"  -d, --debug      turn on debugging messages",
if(WINDOWS) "           and build a debug DLL",

Is there any reason not to give compile_dll() a debug = FALSE/TRUE argument? --debug also turns on debugging messages during the installation process, which might be useful in some cases on Mac too (so --debug has two purposes, print out R level debug messages and set DEBUG=true to build a debug DLL).

Here is one of those messages, but there are a lot: https://github.com/wch/r-source/blob/36e90ea062c4fad5343ae3a4ca122a3b266ad354/src/library/tools/R/install.R#L2372-L2373


If you don't want to do this, that's fine. Adding some documentation / a vignette about this works for me too. I also noticed you can do Sys.setenv(DEBUG = "true"); pkgbuild::compile_dll() and that will work too.

gaborcsardi commented 2 years ago

If you don't want to do this, that's fine.

It already has a debug argument, which even defaults to TRUE, but I suspect that does not do anything on Windows. It could set the DEBUG=true env var on Windows, that would make perfect sense.

Somewhat surprisingly compile_dll() is not used by the pkgbuild::build() at all, so it is not used by pkgload::load_all(), either. Which is good for us now, because we can make it set the env var without affecting lots of people's builds. But it also means it is just better to the env var in the shell, and then it used by load_all(), R CMD INSTALL, pkgbuild::build() etc. and you don't need to remember to call compile_dll() to get a debug build.

DavisVaughan commented 2 years ago

Hmm isn't it used right here? https://github.com/r-lib/pkgload/blob/1c1ce81143a9ae566220c7a9c605320581b9920a/R/load.r#L148

gaborcsardi commented 2 years ago

Yeah, you are right, it is use by load_all(). (But not used by pkgbuild::build() AFAICT.)

That means that if we change it, that will potentially affect many people.

DavisVaughan commented 2 years ago

Ok, I'm fine with documenting DEBUG = true as a Windows + gdb trick somewhere instead. That is more targeted anyways, since we have learned that --debug really does two things (set DEBUG = true AND turn on R level debug messages).

gaborcsardi commented 2 years ago

It is a pity, because DEBUG=true really should create a debug build. OTOH if we change it now, then many people's load_all()-d code will get slower, potentially much slower. And if this happens people will have a hard time finding out why it is suddenly slower.

DavisVaughan commented 2 years ago

What if we changed the default of compile_dll(debug =) to FALSE?

All it does is set -O0 and -g if you don't have a user Makevars file, but I think most of us do, so I doubt it would practically change much?

So then debug = TRUE would:

I guess it would still require calling compile_dll() to get debug = TRUE behavior because pkgload::load_all() doesn't have a debug argument

gaborcsardi commented 2 years ago

Yeah, that's a good idea! That would only affect people who suddenly don't have debug symbols, but that's a better failure mode.

And we could have a workaround for it. E.g. the default could be isTRUE(Sys.getenv("DEBUG", "FALSE")) and then people can just set DEBUG=true on Unix as well. That seems better than adding debug arguments to load_all(), pkgbuild::build(), etc.

gaborcsardi commented 2 years ago

We could probably also add this info to the load_all() message, although that requires some coordination between packages. But it would be nice to see

ℹ Loading pkgbuild (debug build)

or

ℹ Loading pkgbuild (prod build, DEBUG unset)
DavisVaughan commented 2 years ago

The Re-compiling dplyr bit comes from pkgbuild::compile_dll() so that could mention that it is compiling in debug mode

gaborcsardi commented 1 year ago

I actually would like to add -O0 -g -UNDEBUG even if I have a user Makevars file. The user Makevars file is used for every package installation, so I don't want to add debug flags there, but it is good to have a debug build in load_all().

So I think we can keep the default debug = TRUE in compile_dll(), but I'll modify it to