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

CC-1188: add crystal dependency resolution #192

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

CC-1188: add crystal dependency resolution #192

wants to merge 3 commits into from

Conversation

ryan-gang
Copy link
Contributor

No description provided.

Copy link

linear bot commented May 10, 2024

@ryan-gang ryan-gang self-assigned this May 10, 2024
@ryan-gang
Copy link
Contributor Author

After a lot of searching, added this library to the list of dependencies.
https://github.com/crystal-community/kiwi
Might be useful as the datastore they use for their Redis implementation.

@ryan-gang ryan-gang requested a review from rohitpaulk May 10, 2024 08:41
RUN mkdir -p /app-cached/
RUN if [ -d "/app/lib" ]; then mv /app/lib /app-cached/lib; fi

RUN echo "cd \${CODECRAFTERS_SUBMISSION_DIR} && crystal build -p -s -t -o server app/main.cr && sed -i '/^crystal/ s/^/# /' ./spawn_redis_server.sh" > /codecrafters-precompile.sh
Copy link
Member

Choose a reason for hiding this comment

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

Not pertinent to this PR, but just wanted to note: looks like this pattern is becoming commonplace in a lot of our languages. Need to find a better solution. This commenting stuff feels wonky - if a user edits the file it isn't going to be clear to them that there's a hidden script that relies on the file's structure.

@@ -0,0 +1,2 @@
lib/
server
Copy link
Member

Choose a reason for hiding this comment

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

Binaries typically get placed in bin/ or so for most languages - what's the default that Crystal uses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lib/ is where the dependencies are stored.
And server is the final artifact. (We are passing this name with the -o flag).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No other build artifacts would be there in the /app dir.

Copy link
Member

@rohitpaulk rohitpaulk left a comment

Choose a reason for hiding this comment

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

Questions added

@rohitpaulk
Copy link
Member

rohitpaulk commented May 10, 2024

@ryan-gang let's spend some time today thinking about how we can avoid the precompile commenting situation, and how we can make this more transparent to the user (not related to this PR per-se, just a general note)

@ryan-gang ryan-gang requested a review from rohitpaulk May 10, 2024 16:40
@rohitpaulk rohitpaulk removed their request for review May 15, 2024 00:09
@rohitpaulk
Copy link
Member

Gonna keep this on hold for a bit - will revisit soon

@ryan-gang ryan-gang marked this pull request as draft May 19, 2024 09:23
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.

None yet

2 participants