Skip to content

Use cmake build instead builtin #50

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

Closed
wants to merge 1 commit into from
Closed

Use cmake build instead builtin #50

wants to merge 1 commit into from

Conversation

no1seman
Copy link

@no1seman no1seman commented Apr 2, 2022

Seems it's time to move from builtin build type to cmake to make all the stall from the box.

@Totktonada
Copy link
Member

Totktonada commented Apr 4, 2022

I would prefer to use make build type for the sake of simplicity. I have a PoC patch and will share it later to compare. Also I would avoid requirement of the tarantool-devel/dev package (FindTarantool.cmake requires module.h), because it is not actually needed here.

While I'm looking at it, I found that git describe will not work for scm-1 neither with Make, nor with CMake, because luarocks removes the .git directory after cloning the repository. So, it seems, our attempts to lower the version file generation to the luarocks level only works, because they're run within a 'real' repository clone (not one that luarocks doing at luarocks install).

Please, wait a bit and I'll share all findings.

Totktonada added a commit that referenced this pull request Apr 8, 2022
XXX: luarocks removes `.git` directory after cloning a repository. So
actually this approach only works when we're within our cloned
repository. So, questions:

1. We need to reprase comments acoordingly at least.
2. Whether it works with tarantoolctl rocks install in CI?

Now it is possible to obtain a version of the module using the
following code:

```lua
local graphql = require('graphql')
print(graphql.VERSION)
```

It does not work for cloned repository (without calling `tarantoolctl
rocks make`) -- the `graphql.VERSION` field will return `'unknown'`, but
it works for a user scenario: when the module is installed either from
.all.rock file or from the rockspec.

The short list of changes is the following.

- Upload all.rock instead of .src.rock for releases. I don't find a way
  to add a generated file into the .src.rock tarball using luarocks. It
  is not necessary, though: it is completely okay (and even preferable)
  to ship .all.rock for a pure Lua module.
- Upload scm-1 rockspec from CI.
- Moved from the `'builtin'` build type to `'make'` in the rockspec to
  generate the `graphql/VERSION.lua` file at the build stage.
- Added Make targets for luarocks with `graphql/VERSION.lua` file
  generation.
- Added `graphql/init.lua` file with `VERSION` field, which is filled
  from the generated `graphql/VERSION.lua` file if it is present.
  Otherwise it is filled with the `'unknown'` string.

And a couple of minor tweaks:

- Replaced `tarantoolctl rocks new_version --tag $TAG` with `sed` in the
  rockspec publication workflow, because the former strips comments and
  reformats the file. It is a thing of taste: personally I prefer to be
  able to diff rockspecs on the rock server in case of an incident.
- Use `ubuntu-latest` in rockspec publication workflow, because we have
  nothing Ubuntu version specific here. This way it requires less
  maintenance.

The publication workflow now almost the same as in expirationd, see [1].
As for me, it is good to have such code standartized.

This is reimplementation of the original #29 fix from PR #41. The key
points are the following.

- Usage the `'make'` build type allows to lower the logic down to
  luarocks level, so it works not only from CI.
- Complete the implementation by packing into .all.rock instead of
  .src.rock. Otherwise it does not work at all, see [2] for details.
- Generate the gitignored `graphql/VERSION.lua` file (don't touch
  `graphql/init.lua`) to don't leave the repository clone in a dirty
  state.

Also it is the alternative to PR #50, where the `'cmake'` build type was
proposed for the same purposes. The differences are the following.

- Use Make instead of CMake for the same of simplicity.
- Don't require tarantool-dev/devel package for building the rock or
  installing the package from the rockspec.

[1]: tarantool/expirationd#55
[2]: #46 (comment)

Follows up #29
Supersedes PR #50
@Totktonada
Copy link
Member

I followed it up with a proposal in PR #53. Here you can find a brief description of all alternatives and a reason why I prefer to generate the version string from CI. In short: it is simple and explicit.

@Totktonada
Copy link
Member

I pushed my alternative and released graphql-0.1.4.

I'm going to close this pull request, because the problem looks resolved. If you have any inputs, please, let me know.

@Totktonada Totktonada closed this Apr 8, 2022
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.

2 participants