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.

Sign up for GitHub

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

Version watermarking by Dune #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Version watermarking by Dune #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion satysfi.opam
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
opam-version: "2.0"
name: "satysfi"
version: "0.0.5"
maintainer: "gfngfn"
authors: [
"gfngfn"
Expand All @@ -10,6 +9,7 @@ dev-repo: "git+https://github.com/gfngfn/SATySFi.git"
bug-reports: "https://github.com/gfngfn/SATySFi/issues"
build: [
["mkdir" "-p" "temp"]
["dune" "subst"] {pinned}
Copy link
Contributor

@na4zagin3 na4zagin3 Jul 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious. Why is this line executed only when the package is pinned, but not when a non-pinned version is being installed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirmed %%VERSION%% was not substituted when it was installed as a non-pinned package.

# Adding a package pointing to `git+https://github.com/nekketsuuu/SATySFi.git#68f8de124503634b05608f993b693dcf792d437b` to a local repository
$ opam install satysfi
$ satysfi --version
  SATySFi version %%VERSION%%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please read #167 (comment)

This is intended; I think the version string is confusing and not needed for the development period. If needed, use opam pin add.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, Ioverlooked the paragraph. Sorry. So, this PR changes the release process as well: dune-release tag instead of git tag -a. It will be helpful if the new release process is explicitly described somewhere in the repo. Just my two cents.

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, this pull request doesn't change the release process of SATySFi. Just running git tag -a is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it's out of scope of this pull request. I don't intend to change the way to release a new version. dune-release is not fit to current SATySFi.

Copy link
Contributor

@na4zagin3 na4zagin3 Jul 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is a consequence of this PR, which changes how SATySFi should be released.

As OPAM repos prefers a source tarball in url section rather than source repos (especially, the official OPAM repo prohibits source repos there), there are only two ways to substitute watermarks:

  • Release a prepared distribution, in other words, source code preprocessed by dune-subst;
  • Or upload a OPAM file which has a line like ["find" "." "-iname" "*.ml" "-exec" "sed" "-i.bak" "-e" "s/%%VERSION%%/v0.0.6/" "{}" ";"] instead of ["dune" "subst"] {pinned}.

Do you think “dune-release is not fit to current SATySFi” because it automatically attempts to release SATySFi to GitHub and to send a PR to the official OPAM repository?

I edited my previous comment with dune-release distrib that only generates a prepared distribution. Does that make sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, even though it is a consequence, it can be handled by another issue/PR. You're right. Shall I file one?

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed #237 since I don't want to block you on this. Today I learned a lot. Thank you for a fruitful discussion!

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 don't think this pull req changes the current release process, though.... I commented my opinion to that issue #237 (comment). Let's continue this discussion there.

[make "-f" "Makefile" "PREFIX=%{prefix}%"]
]
install: [
Expand Down
3 changes: 2 additions & 1 deletion src/frontend/main.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,9 +948,10 @@ let error_log_environment suspended =
report_error System [ NormalLine(s); ]


(* %%VERSION%% is expanded by "dune subst" *)
let arg_version () =
print_string (
" SATySFi version 0.0.5\n"
" SATySFi version %%VERSION%%\n"
(*
^ " (in the middle of the transition from Macrodown)\n"
^ " ____ ____ ________ _____ ______\n"
Expand Down