Skip to content

perf: Only install dependencies instead of build the package from source when checking #2

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

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

eitsupi
Copy link
Contributor

@eitsupi eitsupi commented Mar 12, 2025

After an update requesting a new Rust version, I noticed that this workflow builds the package twice (and failed).

https://github.com/r-universe/eitsupi/actions/runs/13803060730

I have added a workaround to the package to install the new Rust version using rustup during the Emscripten build1, but obviously there is a normal Linux package build going on here that is difficult to detect.
Perhaps the purpose of this line is to make sure that the dependencies are installable from the R-universe, so isn't this enough?

Footnotes

  1. https://github.com/eitsupi/neo-r-polars/blob/5520c5c3e533ca48b27583e4deae320600f104de/configure#L3-L16

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 13, 2025

@jeroen Could you take a look at this? Thanks!

@jeroen jeroen merged commit be424ad into r-universe-org:master Mar 14, 2025
@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

Let's try it

@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

I don't think that works: https://github.com/r-universe/eitsupi/actions/runs/13803060730/job/38770525533

 > pak::local_install_dev_deps('./neopolars_0.0.0.9000.tar.gz')
  Error: 
  ! error in pak subprocess
  Caused by error in `find_package_root(path = root)`:
  ! Could not find R package in `./neopolars_0.0.0.9000.tar.gz` or its parent directories.

jeroen added a commit that referenced this pull request Mar 14, 2025
@eitsupi eitsupi deleted the dont-build-twice branch March 14, 2025 13:29
@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

Sorry for the mistake, I misunderstood the function of that function.

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

I checked again and the following works, resolving dependencies and displaying them as a table.
Could this be sufficient as it does not actually do the installation but does resolve the dependencies?

pak::local_dev_deps('./neopolars_0.0.0.9000.tar.gz')

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

r-lib/pak#756 (comment)

Something like this could be good.

R -e "pak::pak('deps::${SOURCEPKG}', dependencies = TRUE)"

@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

OK can you send another PR?

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

Sure, #3

@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

Actually I am not sure anymore that there is a problem. The first pak::pak() run builds the package for the native target, i.e. the regular linux binary of your package, that we can load and run on the server.

This is needed for the next step, where we cross compile the package to wasm using emscripten compilers.

In your case compiling the package for regular native Linux already failed, and then it failed again building it for wasm.

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

Surely it is possible that simply removing this line is sufficient?

@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

AFAIR we do need to first build the R package + dependencies for the native platform, before we can build the wasm version. So I think we can't remove it.

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

The following workflow seems to only build once, so I suspect that a pre-build installation is unnecessary.

https://github.com/eitsupi/neo-r-polars/blob/263ccc57c70895294d534ca2ede6ec923dbad93f/.github/workflows/build-wasm.yml

@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

I think it depends on the package. Some R packages need to run something while they are built. In this case a working local version of the package must be available first, as we cannot execute wasm binary code on linux.

It could be something has changed, but I need to look into it more carefully.

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

Thanks for the clarification.
In that case, I think the current code makes sense.

I'll think of a different way to deal with the problem of the Rust toolchain being outdated.

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 14, 2025

How about not terminating the job when the first build fails?
Like:
https://github.com/r-universe/eitsupi/actions/runs/13803060730/job/38770524486

@jeroen
Copy link
Member

jeroen commented Mar 14, 2025

I'll think of a different way to deal with the problem of the Rust toolchain being outdated.

That seems like a different problem? Can we just update it?

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 15, 2025

I'm sorry I didn't give you any background information.
The original problem is that Rust on the rwasm container is not being updated.

I've made the configure script exec rustup to install a new toolchain when it detects Emscripten in the configure script, but here the first build is not Emscripten so it's not enough and the build fails.

In other words, during a normal Linux build on R-universe, rustc that continues to be updated is used, but rustc in the container is frozen, so if the MSRV is newer, the build will fail.
Perhaps it's worth detecting that the package requires cargo and updating Rust in that case?

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