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

Makefile & Feature Installation instructions #133

Conversation

AshtonStephens
Copy link
Collaborator

Use the following template to create your pull request

Description

  • Changes some package names to be more idomatic. emily-operation-lambda -> emily-lambda, etc.
  • Change smithy generated source locations so clean command doesn't wipe build.rs file.
  • Updates the local tools required to run the repository in README.md
  • Adds a Makefile with some emily related commands and general build command.

Note that the Makefile is not complete with all the tools for all the monorepo packages. We should update the Makefile with the commands necessary to develop for the packages as we go.

@AshtonStephens AshtonStephens added the documentation Improvements or additions to documentation label May 8, 2024
@AshtonStephens AshtonStephens requested review from hstove and netrome May 8, 2024 14:33
@AshtonStephens AshtonStephens self-assigned this May 8, 2024
@AshtonStephens AshtonStephens linked an issue May 8, 2024 that may be closed by this pull request
3 tasks
@AshtonStephens AshtonStephens requested review from 8marz8 and removed request for hstove May 8, 2024 14:33
@AshtonStephens AshtonStephens changed the title 100 feature installation instructions for build dependencies in readmemd Makefile & Feature Installation instructions May 8, 2024
@AshtonStephens AshtonStephens marked this pull request as draft May 8, 2024 15:43
@AshtonStephens
Copy link
Collaborator Author

Converted to draft until I fix the CI.

Copy link
Contributor

@8marz8 8marz8 left a comment

Choose a reason for hiding this comment

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

LGTM. I was able to build it successfully following the readme. Just needed to update my CommandLineTools and Node.js (Got clear errors)

@AshtonStephens AshtonStephens force-pushed the 100-feature-installation-instructions-for-build-dependencies-in-readmemd branch 5 times, most recently from 677c130 to c42e554 Compare May 8, 2024 18:49
@AshtonStephens AshtonStephens marked this pull request as ready for review May 8, 2024 18:51
@AshtonStephens AshtonStephens requested review from hstove and removed request for netrome May 8, 2024 18:53
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

It works on my machine 👍

@netrome
Copy link
Contributor

netrome commented May 8, 2024

Though one annoying point: pnpm-lock.yaml seems to change whenever I run either make install or make test.

Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

LGTM! The makefile commands work as expected and are helpful.

One note is that, with pnpm, the package-lock.json files shouldn't be being updated (and should be deleted actually). That could indicate you're using both npm install and pnpm install. The pnpm-lock.yaml file at the root of the workspace acts as the lockfile for all workspace packages.


- run: cargo test
- name: Run tests
run: make test
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just update this Github workflow to be something like "Run tests", since this will run both rust and JS tests? I'd be fine with that, and if there are Rust / Contract / etc workflows we want separately we can do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'll change this to just be run tests and keep the contracts one because it looks like it does more than just this does. Ideally these wouldn't need to be separate because GitHub workflowss aren't really for testing, they're for preventing regression, and so long as the workflow is blocked when we mess something and we cursorily know why then we're good.

emily/api-definition/package.json Outdated Show resolved Hide resolved
emily/api-definition/package.json Outdated Show resolved Hide resolved
@AshtonStephens
Copy link
Collaborator Author

Though one annoying point: pnpm-lock.yaml seems to change whenever I run either make install or make test.

If you try committing that change locally and then rerunning make install && make test does it change further?

@hstove
Copy link
Contributor

hstove commented May 8, 2024

@netrome if you run pnpm -v what version do you have? If it's not 9.x.x, that could be causing the lockfile to change, but I'm not sure.

@hstove
Copy link
Contributor

hstove commented May 8, 2024

Ah, yeah the current pnpm-lock file was generated with pnpm version 6, which is why the lockfile is changing. Happens for me too when I run pnpm install after checking out the repo.

@hstove
Copy link
Contributor

hstove commented May 8, 2024

I've opened #135 which should prevent accidental usage of different PNPM versions

@AshtonStephens AshtonStephens force-pushed the 100-feature-installation-instructions-for-build-dependencies-in-readmemd branch from c42e554 to 8a966f8 Compare May 8, 2024 19:25
@AshtonStephens AshtonStephens force-pushed the 100-feature-installation-instructions-for-build-dependencies-in-readmemd branch from 8a966f8 to 43db88b Compare May 8, 2024 19:37
@AshtonStephens AshtonStephens merged commit 1ea49bd into main May 8, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Installation Instructions for Build Dependencies in README.md
4 participants