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

Improve documentation #708

Merged
merged 23 commits into from May 17, 2017
Merged

Improve documentation #708

merged 23 commits into from May 17, 2017

Conversation

tindzk
Copy link
Contributor

@tindzk tindzk commented May 7, 2017

A couple of suggestions for further improvements:

  • We should mention that Scala Native is garbage-collected and uses the Boehm GC. Does it make sense to include it as a core feature in index.rst?
  • We should have a section on compatibility with Scala JVM, notably which features are not supported yet.
  • The qualified native.<Type> access slightly impairs the reading flow. We could mention that all examples expect a prior import scala.scalanative.native._.
  • Using dots in enumerations is not very common and in some cases may add some confusion (see "Compilation modes" on the sbt page). I would suggest to remove them everywhere.

* installation of LLVM and Clang
* installation of Boehm GC
* Java 8 or later
* sbt 0.13.15
Copy link
Member

Choose a reason for hiding this comment

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

The sbt version doesn't really matter, since sbt will load the one defined in project/build.properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true. What do you think about removing the mention of project/build.properties from the sbt chapter? If we specify it in the setup chapter, it would be redundant.

Copy link
Member

Choose a reason for hiding this comment

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

The project/build.properties file ensures that the build is reproducible across different system so it should always be created. http://www.scala-sbt.org/0.13/docs/Basic-Def.html#Specifying+the+sbt+version


Installing LLVM, Clang and Boehm GC
-----------------------------------
LLVM and Boehm GC are Scala Native's only external dependencies. Here are
Copy link
Member

Choose a reason for hiding this comment

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

I've also worked on updating this section to reflect that the Scala Native runtime and JDK implementation now depends on both Boehm GC and the RE2 regular expression engine.

https://github.com/scala-native/scala-native/pull/706/files#diff-a2d0368ffcd2931da4849e9d4288854e

Great work, BTW 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't see your pull request. Let's get yours merged first, then I'll resolve any conflicts on my end.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, lets merge @jonas' PR first, it's a bit smaller.

Copy link
Contributor

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

Thanks, that's a great improvement!

In the section Finding the right signature, it may be interesting to add something about getting POSIX signatures right. For instance, look at types.h. It re-defines some of the types specified by POSIX to use the largest type that we encounter in macOS or Linux, and structs that use these types must be re-defined to use the new definitions, as in stat.c for instance.

I can take care of that last part if you prefer.

C compilers typically require to pass an additional ``-l mylib`` flag to
dynamically link with a library. In Scala Native, one can annotate libraries to
link with using the ``@native.link`` annotation. As in C, it takes the library
name without the ``lib`` prefix.
Copy link
Contributor

Choose a reason for hiding this comment

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

As in C, it takes the library name without the lib prefix.

I'm not sure this sentence makes things much clearer. I got a bit confused because lib is a suffix in the example above, plus this is not always true (take for instance zlib, you need to pass -l z to the linker). Maybe just say that you have to put whatever you would give your C compiler? I'm not a native English speaker, so that's really just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This was in reference to libuv, which becomes -luv (later in the section there is an example with it). I will rephrase this sentence to make it clearer.

@densh
Copy link
Member

densh commented May 9, 2017

Please rebase on top latest master due to conflicting docs changes.

@tindzk
Copy link
Contributor Author

tindzk commented May 14, 2017

@Duhemm It is a good idea to mention this approach somewhere in the documentation, but I would suggest to do this in a separate chapter (or section) instead. From my understanding, this is only relevant for porting system libraries (such as POSIX). The end-user libraries I encountered all used platform-independent types such as size_t.

@densh I accidentally did a merge instead of a rebase. The conflicts were resolved.

@densh
Copy link
Member

densh commented May 16, 2017

@Duhemm It would be indeed great to document that approach, but I have an impression this is out of scope of this PR. It mostly cleans up the content that we have now. Your suggested change should probably be addressed in subsequent iterations of the docs.

Copy link
Member

@densh densh left a comment

Choose a reason for hiding this comment

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

See comments below.

* installation of LLVM and Clang
* installation of native libraries
* Java 8
* sbt
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to mention version here, i.e. sbt 0.13.x

* installation of native libraries
* Java 8
* sbt
* LLVM 3.7
Copy link
Member

Choose a reason for hiding this comment

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

3.7 or newer.

* installation of sbt
* installation of LLVM and Clang
* installation of native libraries
* Java 8
Copy link
Member

Choose a reason for hiding this comment

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

Java 8 or newer.

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 wrote in the paragraph before "Scala Native has the following minimum system requirements". Should I still add "or newer" for clarity?

Copy link
Member

@densh densh May 17, 2017

Choose a reason for hiding this comment

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

I'd say it would be more clear to write Java 8+ or something along those lines.

relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC

It is likely that the ``LDFLAGS`` environment variable enables hardening. For example, this occurs when the ``hardening-wrapper`` package is installed on Arch Linux. It can be safely removed.

Copy link
Member

@densh densh May 16, 2017

Choose a reason for hiding this comment

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

I have an impression that this one deserves to be on the same page together with FAQ, given that we don't expect many people to hit this problem

@densh densh merged commit d034230 into scala-native:master May 17, 2017
@densh
Copy link
Member

densh commented May 17, 2017

LGTM, thanks for the clean up! 😄 👍

@tindzk tindzk deleted the docs branch May 17, 2017 18:27
@tindzk
Copy link
Contributor Author

tindzk commented May 17, 2017

Great, thanks a lot! Do you have any comments to my suggestions from the first post?

@densh
Copy link
Member

densh commented May 18, 2017

  • Regarding GC: we're going to have a new garbage collector soon, we'll probably put into highlights on the first page once it happens. :)
  • Regarding compatibility: we have javalib and language pages, the combinations of the two should give a reasonable impression of what's there and what's not.
  • Regarding having an import: I am not 100% convinced, but I understand the idea. We can do it if you feel like it's going to improve the docs for newcomers.
  • Dots in enumeration: sure, lets change that.

muxanick pushed a commit to muxanick/scala-native that referenced this pull request May 25, 2017
* docs: Fix spelling of Nix and NixOS

* docs: Improve formatting of operating systems

* docs: Use `apt` instead of `apt-get` on Ubuntu

* docs: Fix sbt instructions

* docs: Use inline literals in interoperability table

* docs: Use footnotes in interoperability table

* docs: Add instructions for Arch Linux

* docs: Improve wording in environment setup

We should mention it in a separate chapter that Scala Native uses
Clang.

* docs: Add missing section on function pointers

* docs: Improve section on linking

* docs: Don't specify sbt version

* docs: Rephrase remark about `lib` prefix

* docs: Fix preposition on interoperability page

* docs: Add missing types to interoperability table

* docs: Improve wording of section on unchecked casts

* docs: Add section on platform-specific types

* docs: Add section on determining the size of types

* docs: Specify version for sbt

* docs: Move section on troubleshooting to FAQ

* docs: Fix enumeration on sbt page

* docs: Improve punctuation and grammar on sbt page

* docs: Clarify version numbers
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

4 participants