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

Replace Rakefile with Makefile #537

Merged
merged 8 commits into from Aug 15, 2022
Merged

Replace Rakefile with Makefile #537

merged 8 commits into from Aug 15, 2022

Conversation

paulbellamy
Copy link
Contributor

@paulbellamy paulbellamy commented Aug 12, 2022

This does a few things:

  • Replace Rakefile with Makefile (to match https://github.com/stellar/rs-stellar-xdr).

  • Remove all ruby. Not needed anymore. xdrgen is dockerized

  • Automate and dockerize the dts-xdr build step.

  • Split xdr directory into xdr/curr and xdr/next. Which one it uses is hard-coded in src/xdr.js and types/xdr.d.ts for now.

  • Cleaned up the readme for all of the above.

  • Incidentally, this updated the curr xdr to latest core master. But could leave that out of this PR if desired.

The main potential issue with this are our hand-edit hacks in the .d.ts files. So like.. make can re-generate those files, but... They'll be broken until you copy-pasta over the changes again. oof.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

This looks good to me. How will people choose current vs next in the SDK? We wouldn't want to release the next XDR, as it is very unstable. Or rather, we'd only want to release next on non-regular release numbers, like maybe a pre-release, or something else.

@paulbellamy
Copy link
Contributor Author

paulbellamy commented Aug 15, 2022

Yeah, my plan for now for dev was to do a separate branch and switch it in there. "Proper" switching between the xdrs is tbd (any suggestions?), but figured backporting this would minimize the diff later.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Incidentally, this updated the curr xdr to latest core master. But could leave that out of this PR if desired.

If doing this is not inconvenient, could we leave that out of this PR so that this change is isolated to a refactor only, then follow up with another PR where we generate it?

Otherwise 👍🏻.

@paulbellamy paulbellamy merged commit 9fba305 into master Aug 15, 2022
@paulbellamy paulbellamy deleted the soroban branch August 15, 2022 16:19
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

was out sick so late to the party but figured I'd add my thoughts anyway

Stellar-overlay.x \
Stellar-transaction.x \
Stellar-types.x
XDR_FILES_LOCAL_CURR=$(addprefix xdr/curr/,$(XDR_FILES_CURR))
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole curr and next thing broke the entire ecosystem, I bet. Why couldn't they keep xdr/ and add xdr-next/??

cd .. && rm -rf dts-xdr
yarn run prettier --write types/xdr.d.ts
```
1. Make sure you have [Docker](https://www.docker.com/) installed and running.
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -7,7 +7,6 @@
"scripts": {
"test": "gulp test:node",
"test:watch": "gulp test:watch",
"xdr": "bundle exec xdrgen -o src/generated -l javascript -n stellar-xdr xdr/Stellar-ledger-entries.x xdr/Stellar-ledger.x xdr/Stellar-overlay.x xdr/Stellar-SCP.x xdr/Stellar-transaction.x xdr/Stellar-types.x",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add the make reset-xdr command here?

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

3 participants