Skip to content

Conversation

@RReichert
Copy link
Contributor

@RReichert RReichert commented Feb 3, 2025

Description

Fix the docker build

See: https://github.com/swift-nav/docker-recipes/pull/439

API compatibility

No compatibility issues

API compatibility plan

N/A

@RReichert RReichert requested a review from a team as a code owner February 3, 2025 09:48
@RReichert RReichert changed the title Update NodeJS version Update NodeJS + Haskell version Feb 3, 2025
Dockerfile Outdated
RUN \
if [ "$(ls /tmp)" ]; then ls /tmp; false; fi \
&& stack install --resolver lts-10.10 sbp \
&& stack install --resolver lts-23.7 sbp \
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, I think I tried it like this in the past, but it didn't work. I remember you did it like this previously. Can you double-check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand, the Dockerfile explicitly states what version it wants to use to build the SBP version, where as in the Haskell test build it uses whatever is in that haskell/stack.yaml file. I don't know why there is a difference there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've since reverted this change and tried to unify the Haskell version to lts-14.9

@RReichert RReichert marked this pull request as draft February 4, 2025 02:40
RUN \
if [ "$(ls /tmp)" ]; then ls /tmp; false; fi \
&& stack install --resolver lts-10.10 sbp \
&& stack install --resolver lts-14.9 sbp \
Copy link
Contributor Author

@RReichert RReichert Feb 4, 2025

Choose a reason for hiding this comment

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

keeping the same version as Haskell build stage

resolver: lts-14.9

we currently need a version that is greater than v12

Comment on lines -85 to -89
warnings.simplefilter("always")
# Check for warnings.
assert len(w) == 1
assert issubclass(w[0].category, RuntimeWarning)
assert str(w[0].message).startswith('Bad message parsing for line')
Copy link
Contributor Author

@RReichert RReichert Feb 4, 2025

Choose a reason for hiding this comment

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

This is a strange test and is dependent on the internal implementation of the python JSON library. I've removed it since its not really testing anything important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to regenerated the nodejs list of packages, the old versions were unsupported with the latest LTS

Comment on lines +17 to +23
"assert": "^2.1.0",
"binary-parser": "^1.7.0",
"cuint": "^0.2.2",
"js-yaml": "^3.13.1",
"node-int64": "^0.4.0"
"node-int64": "^0.4.0",
"path": "^0.12.7",
"stream": "^0.0.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new packages were required to generate javascript/sbp.bundle.js

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 5, 2025

ARG DEBIAN_FRONTEND=noninteractive

ENV NODE_VERSION=v18.17.0
ENV NODE_VERSION=v22.13.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the latest NodeJS TLS

imagemagick \
enchant \
clang-format-6.0 \
python3.7 python3.7-dev python3.7-distutils \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python 3.7 is no longer working, the build keep reporting:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python 3.7 and python 3.8 are now at EOL

python3 python3-dev python3-distutils \
python3.9 python3.9-dev python3.9-distutils \
python3.10 python3.10-dev python3.10-distutils \
python3.11 python3.11-dev python3.11-distutils \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've included python 3.11 since its the latest python version that the Ubuntu Focal offers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to regenerate after updating the NodeJS version

Comment on lines -26 to -49
[coverage:run]
branch = True
include = sbp/*
parallel = True

[coverage:report]
exclude_lines =
# Have to re-enable the standard pragma
pragma: no cover

# Don't complain about missing debug-only code:
def __repr__
if self\.debug

# Don't complain if non-runnable code isn't run:
if 0:
if __name__ == .__main__.:
ignore_errors = True

[coverage:omit]
omit =
tests/*
limbo/*
data/*
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 was the source of a lot of headaches. The python tests would randomly crash because of some internal Python code coverage error, it would normally happen with Python 3.10 builds, but not exclusively:

image

There is no need for code coverage here, we are looking to run the test cases and see if they work. If we need code coverage, we can add it later with SonarCloud integration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe someone else from @swift-nav/algint-team can confirm that we don't require coverage right now? That would be my only concern.

@RReichert RReichert marked this pull request as ready for review February 5, 2025 21:03
@RReichert RReichert requested review from a team and sbmueller and removed request for sbmueller February 6, 2025 01:46
Copy link
Contributor

@sbmueller sbmueller left a comment

Choose a reason for hiding this comment

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

No feedback received on python coverage, therefore approving

@RReichert RReichert merged commit 0f4434b into master Feb 10, 2025
12 checks passed
@RReichert RReichert deleted the rodrigor/fix-nodejs-version branch February 10, 2025 10:56
@RReichert RReichert mentioned this pull request Feb 10, 2025
RReichert added a commit that referenced this pull request Feb 10, 2025
# Changes

After updating the following PRs:

* #1470
* swift-nav/docker-recipes#439

I can finally update the docker image here to fix both repositories.
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