Skip to content

Conversation

homersimpsons
Copy link
Contributor

@homersimpsons homersimpsons commented Jul 6, 2025

Before: 436MB, after: 187MB (57% reduction)

There are 160MB from the base node:lts-alpine

Missing potential savings:

  • A potential improvement on our side would be to avoid having to install dependencies globally and locally (I do not know why this is necessary, but it clearly fails when we don't do this). Alternatively it is maybe possible to create a symbolic link between local and global dependencies.
  • Another saving would be to extract the deps we need from the open-abap-core repository and clean it up. We could maybe even save runtime doing so by preparing the directory layout to run the tests.
    execSync(`cp open-abap-core/src/unit/*.clas*.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/exceptions/* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/rtti/* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/abap/abap.type.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/abap/cl_abap_char_utilities.clas.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/cl_message_helper.clas.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/abap/math/cl_abap_math.clas.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/kernel/kernel_internal_name.clas.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    // DDIC, avoid copying transparent database table artifacts
    execSync(`cp -r open-abap-core/src/ddic/dtel/timestamp* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp -r open-abap-core/src/ddic/dtel/sy* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp -r open-abap-core/src/ddic/dtel/int1* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp -r open-abap-core/src/ddic/dtel/sotr_conc* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp -r open-abap-core/src/ddic/ttyp/string_table* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp -r open-abap-core/src/ddic/structures/scx* ${this.tmpDir}/deps/`, {stdio: 'pipe'});
    execSync(`cp open-abap-core/src/classrun/*.intf.abap ${this.tmpDir}/deps/`, {stdio: 'pipe'});

There are 2 commits:

  1. The first one is an update to the test results that was necessary because the open-abap-core had changes
  2. The second one is the one where I shrink the size

@homersimpsons homersimpsons requested a review from a team as a code owner July 6, 2025 20:26
@homersimpsons homersimpsons force-pushed the chore/shrink-docker-image branch from 70c0824 to c9176a0 Compare July 6, 2025 20:27
@homersimpsons
Copy link
Contributor Author

homersimpsons commented Jul 6, 2025

For some reason the tests are failing with TypeError: abap.statements.clear is not a function but tests in docker are passing on my side. I'm able to reproduce the issue in github codespace though.

Both bin/run-tests.sh and bin/run-tests-in-docker.sh works correctly. But test.ts just seems to invoke bin/run.sh (which is also invoked by the other scripts). Running it directly gives the correct result too: bin/run.sh simple-pass /workspaces/abap-test-runner/tests/simple-pass /workspaces/abap-test-runner/output

Interestingly the compilation differs between both sides: diff_npm-test_left_bin_run_right.diff.txt, in particular there is this following diff (that is maybe the issue?):

diff -r /tmp/exercism-abap-runner-r9dwHl/compiled/cl_abap_elemdescr.clas.mjs /tmp/exercism-abap-runner-ta6Pbp/compiled/cl_abap_elemdescr.clas.mjs
[...]
<       abap.statements.clear(ls_row);
---
>       ls_row.clear();

There is 0 diff in process.env between both invocations, nor in the arguments.

Even if I create a simple main.js file and execude it with node main.js I have the correct results.json:

import { spawnSync } from "child_process";
const res = spawnSync('bash', ['bin/run.sh', 'simple-pass', '/workspaces/abap-test-runner/tests/simple-pass', '/workspaces/abap-test-runner/output'], {cwd: '/workspaces/abap-test-runner'});

I tried with execSync instead of spawnSync in tests without success. I tried to swap mocha with jest without any success.

@larshp sorry for the ping, if you have any idea of what could possibly go wrong I would be happy to test them. I do not know ABAP, and I cannot understand why the transpilation are different between both invocations.

@homersimpsons homersimpsons force-pushed the chore/shrink-docker-image branch 2 times, most recently from d69e2d4 to 6e7d624 Compare July 6, 2025 21:09
@IsaacG
Copy link
Member

IsaacG commented Jul 7, 2025

The tests need to pass and this PR needs an approval from an ABAP maintainer.

@larshp
Copy link
Member

larshp commented Jul 8, 2025

maybe I just broke stuff, #229 opened, try rebasing after its merged

@homersimpsons homersimpsons force-pushed the chore/shrink-docker-image branch from 7643346 to c676334 Compare July 8, 2025 11:04
@homersimpsons
Copy link
Contributor Author

maybe I just broke stuff, #229 opened, try rebasing after its merged

Okay thanks! Everything should be ready now.

@homersimpsons homersimpsons requested a review from larshp July 8, 2025 11:05
@larshp
Copy link
Member

larshp commented Jul 8, 2025

🚢

@larshp
Copy link
Member

larshp commented Jul 8, 2025

thanks

@larshp larshp merged commit eadcf35 into exercism:main Jul 8, 2025
1 check passed
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.

3 participants