Add typer target #309

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants

This patch adds a typer target to the Makefile. It makes it easier for developers to start adding specs to the codebase. It also has a build_plt target with the built-in applications typer needs to analyze rebar.

Contributor

ferd commented Jul 7, 2014

I don't see too much that can hurt with this. I'm open to getting this in. @tsloughter @tuncer?

Contributor

tuncer commented Jul 7, 2014

We should change make build_plt to write the file to ~/.dialyzer_OTPVSN_rebar_plt or ~/.dialyzer_rebar_OTPVSN_plt and adapt make dialyzer accordingly.

On the topic of adding specs all over, AFAIR, @dizzyd wasn't too keen on doing that. But exported functions should have type specs.

varnerac commented Jul 8, 2014

Updated to address the concerns raised by @tuncer

Contributor

tuncer commented Jul 8, 2014

@NineFX wrote:

Updated to address the concerns raised by @tuncer

Thanks. I'm not sure if the list of apps to include was smaller before, but it's long enough to require listing each app on an individual line. That makes it much easier to edit.

Contributor

ferd commented Jul 8, 2014

I believe building the PLT will fail in older Rebar releases that didn't contain apps such as diameter to begin with (I don't understand why that app is in there to begin with).

We're also gonna have to test with old versions and see if all the options are there, but yeah.

jj1bdx commented Jul 9, 2014

$(shell ... ) is a GNU Make dialect (i.e., not working on BSD Make)

Contributor

varnerac-ubnt commented Jul 9, 2014

Ok, do you want the per OTP version number in the PLT file or portability between GNU and BSD make? The current Makefile already uses the GNU shell command:
https://github.com/rebar/rebar/blob/master/Makefile#L32

Contributor

tuncer commented Jul 9, 2014

Yes, rebar's Makefile is already unportable and this patch's use of GNU Make syntax is not a merge blocker.

It's not exactly the same thing, but we can fix it later as follows:

`OTPVSN=@`sh -c "erl -eval '$(VSNCMD)' -noshell"`

However, that could be a problem for Windows portability, so let's defer the issue for now. Filed #318.

Makefile
@@ -2,6 +2,9 @@
REBAR=$(PWD)/rebar
RETEST=$(PWD)/deps/retest/retest
+VSNCMD=io:fwrite("~s",[erlang:system_info(otp_release)]), halt().
@tuncer

tuncer Jul 9, 2014

Contributor

Should this be quoted?

@varnerac-ubnt

varnerac-ubnt Jul 9, 2014

Contributor

It "works for me" (OS X). Does it work for you?

@tuncer

tuncer Jul 15, 2014

Contributor

It works with GNU make, but I haven't tried with a BSD make.

Makefile
+ -@dialyzer --build_plt --output_plt $(PLT_FILENAME) --apps erts kernel \
+ stdlib crypto compiler asn1 diameter eunit tools ssl edoc reltool \
+ snmp sasl
+
@tuncer

tuncer Jul 9, 2014

Contributor

Let's put the apps on individual lines for easier editing.

varnerac commented Jul 9, 2014

@tuncer Updated with one app per line.

Contributor

ferd commented Jul 15, 2014

So the make build_plt command fails on all older rebar installs where diameter doesn't exist, because, well, the diameter compiler doesn't exist.

Are we fine with this command not working on older versions we officially support (and the diameter compiler not working there either)? I think I'd probably prefer it if we either didn't have diameter in there or did a thing like a second command with --add_to_plt for diameter that can be allowed to fail silently or work accordingly.

@ferd I'd prefer the first option and just add diameter later rather than monkey with the Makefile too much. Shall I update the PR now or are you waiting for consensus?

Contributor

ferd commented Jul 15, 2014

I'd prefer not to typecheck diameter than not have the command work on earlier versions for the time being, personally. I expect more bug reports of people who can't make it work than bug reports discovered through diameter scans.

Contributor

tuncer commented Jul 15, 2014

erlang:system_info(otp_release) won't give us the patch level. So 17.1.2 will be 17. Isn't this a problem?

Makefile
@@ -21,12 +25,31 @@ check: debug xref dialyzer deps test
xref:
@./rebar xref
+build_plt:
+ -@dialyzer --build_plt --output_plt $(PLT_FILENAME) --apps \
@tuncer

tuncer Jul 15, 2014

Contributor

Do we really want to ignore the exit code here?

@varnerac

varnerac Jul 15, 2014

Yes. The exit code on the dialyzer is usually non-zero. For example, this build_plt target on R16B03-1 yields:

ninefx-mbp:rebar nine$ dialyzer --build_plt --apps  erts  kernel  stdlib  crypto  compiler  asn1  eunit  tools  ssl  edoc  reltool  snmp sasl
  Compiling some key modules to native code... done in 0m25.09s
  Creating PLT /Users/nine/.dialyzer_plt ...
eunit_test.erl:305: Call to missing or unexported function eunit_test:nonexisting_function/0
Unknown functions:
  dbg:ctp/1
  dbg:p/2
...
Unknown types:
  erl_syntax:syntaxTree/0
  inet:ipaddress/0
...
 done in 1m18.69s
done (warnings were emitted)
ninefx-mbp:rebar nine$ echo $?
2

It's not really an error that the user should worry about. They'll still have the text output to make an informed decision. I typically write my dialyzer to depend on build_plt. It doesn't rebuild it every time, but checks that it's up to date. I use it in my precommit target where I execute:

distclean get-deps build-deps ct edoc dialyzer

to make sure I didn't break any portion of the project.

If we don't ignore the return code, which I think we all agree isn't an "error", we can't depend on the target elsewhere in the Makefile nor chain targets together from the commandline.

@tuncer

tuncer Jul 15, 2014

Contributor

Makes sense.

@varnerac

varnerac Jul 15, 2014

Would a precommit target for rebar make sense? It may help prevent issues like the dialyzer warnings that pop up under Erlang 17+.

@tuncer How do you want to do the version detection?

http://www.erlang.org/doc/system_principles/versions.html

Contributor

tuncer commented Jul 15, 2014

If erlang:system_info(otp_release) is a number

file:read_file(filename:join([code:root_dir(), "releases", erlang:system_info(otp_release), "OTP_VERSION"]))

otherwise

erlang:system_info(otp_release)
Contributor

ferd commented Jul 15, 2014

You could detect features rather than versions. Try to add diameter but let it silently fail if it couldn't. That sounds like it does exactly what we'd want, but would require a second --add_to_plt command to be run after the first one.

@ferd How about just using make's ignore errors prefix in the build_plt target:

-dialyzer --build_plt --output_plt $(PLT_FILENAME) --apps \
 erts \
...
-dialyzer --add_to_plt --output_plt $(PLT_FILENAME) --apps diameter

It will just fail quietly. Presumably any developer building a PLT file on an old version of Erlang will understand why the second command failed. The error won't prevent other targets from executing.

Contributor

ferd commented Jul 15, 2014

That would be fine by me, yes.

@ferd The build_plt changes are in place for diameter
@tuncer I've added an encrypt that captures the version

Contributor

tuncer commented Jul 16, 2014

How about merging #327 (fix for #326) first and then calling rebar_utils:otp_release/0 in the Makefile?

Contributor

varnerac-ubnt commented Jul 16, 2014

Sure. I'm traveling now but I'll look at that when I get home.

Contributor

tuncer commented Jul 17, 2014

It seems like you misinterpreted my comment. The idea was to merge #327 to master, rebase your patch on top of that, and call the new internal function in your patch. 0cd3be8 and bf5e67a are not the right thing to do in this case.

Contributor

tuncer commented Jul 22, 2014

@NineFX, ping? This needs to be rebased to have a single commit modifying Makefile. Otherwise, the new Makefile looks fine to me, and @ferd was also okay with the latest version.

varnerac-ubnt added some commits Jun 27, 2014

Add typer target
This patch adds a typer target to the Makefile. It makes it
easier for developers to start adding specs to the codebase. It
also as a build_plt target with the built-in applications typer
needs to analyze rebar.

This also includes the Erlang version in the generated PLT file.
typer and dialyzer targets use this file. Finally, the distclean
target removed the versioned PLT file.
Merge branch 'master' of https://github.com/rebar/rebar into typer
Conflicts:
	src/rebar_utils.erl
	test/rebar_otp_release_tests.erl

Should I leave merge message or squash?

Contributor

tuncer commented Jul 23, 2014

It's best to have a single commit: Add typer target.

Contributor

tuncer commented Aug 7, 2014

@NineFX ping?

Contributor

varnerac-ubnt commented Aug 7, 2014

I can't figure out how I squash the merge message in git

Contributor

tuncer commented Aug 7, 2014

@varnerac-ubnt wrote:

I can't figure out how I squash the merge message in git

You need to revert to an earlier state or manually back out the non-Makefile changes before rebasing your branch. You can:

  1. use git reset, rewrite the branch, and force push
  2. or create a fresh branch to overwrite the old one and force push
  3. or create a new pull request with just the new Makefile
Contributor

tuncer commented Aug 25, 2014

ping?

I'd like to get this in, so if you don't have time, I'll submit a rebased version of your branch.

Contributor

tuncer commented Aug 28, 2014

Here's a rebased version: #352

ferd added a commit that referenced this pull request Aug 28, 2014

Contributor

ferd commented Aug 28, 2014

Merged the rebased version.

@ferd ferd closed this Aug 28, 2014

@varnerac varnerac deleted the varnerac:typer branch Oct 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment