adamdruppe / arsd

This is a collection of modules that I've released over the years. Most of them stand alone, or have just one or two dependencies in here, so you don't have to download this whole repo.
http://arsd-official.dpldocs.info/arsd.html
531 stars 128 forks source link

module is in file which cannot be read #194

Closed ghost closed 5 years ago

ghost commented 5 years ago

Using dub.json:

{
   "dependencies": {"arsd-official": "~>2.2.1"}
}

and app.d:

import arsd.http2;

Result:

$ dub build
Performing "debug" build using C:\dmd2\windows\bin\dmd.exe for x86_64.
etc ~master: building configuration "application"...
source\app.d(3,8): Error: module `http2` is in file 'arsd\http2.d' which cannot
be read
adamdruppe commented 5 years ago

I typically say use the subpackages instead:

http://code.dlang.org/packages/arsd-official%3Ahttp

but truth be told I barely support dub. The way I actually recommend using my libs is to simply download the modules you need to your own build.

I don't know what the problem here is, the http2.d module is obviously there.

andre2007 commented 5 years ago

ARSD Dub.json only adds the modules as sub packages but not as dependencies, therefore just having dependency to arsd-official is not sufficient. You need to have dependency "arsd-official:http". Yes, subPackage http references module http2.d.

adamdruppe commented 5 years ago

You could also get openssl for windows... I know I have a copy somewhere but I can't find it right now.

eventually btw I'd love to get off openssl on windows and use schannel instead. if someone were to pull request that i would be glad :P

but still if i can find those files i will provide them somewhere.... just in the mean time "openssl windows" on web search should find something.

andre2007 commented 5 years ago

@cup Just add this line to dub.json "versions": ["without_openssl "]

ghost commented 5 years ago

@andre2007 no:

$ dub build
Performing "debug" build using C:\dmd2\windows\bin\dmd.exe for x86_64.
etc ~master: building configuration "application"...
C:\Users\Steven\AppData\Local\dub\packages\arsd-official-2.2.1\arsd-official\
http2.d(594,16): Error: undefined identifier SslClientSocket
andre2007 commented 5 years ago

To fully enable the ARSD file structure, dub needs to be enhanced to use the dmd -mv switch. https://github.com/dlang/dub/issues/1728

ghost commented 5 years ago

@andre2007 that seems convoluted, wouldnt it be better for @adamdruppe to change the structure to match Dub convention, rather than the other way around?

If so what would that look like?

andre2007 commented 5 years ago

@cup this seems to be a bug in http2.d. If I remember correctly Adam already solved it in master but hasn't released a new version and therefore the dub package still contains the bug.

Regarding your last comment, I doubt Adam will change the file layout. He wrote the library for his purpose with a structure suitable for him. He was really kind to open source the library and even add a dub.json.

I need to do some tests but I think it would work even without dub enhancement. On every dub sub package definition of the arsd dub.json s.th. like this has to be added: "dflags": ["-mv='arsd.http2=http2.d'] If this works, the targetType of each subPackage can be set to library. As last step, in dub.json root, dependencies to each subPackages can be defined. This would solve all issues. Of course the dub enhancement would be much cleaner than directly add -mv arguments.

One blocker is, there are some modules in the library which throwing c style deprecation errors or errors like "line not reachable".

adamdruppe commented 5 years ago

well I'll add a git tag if it is just version stuff.

What deprecation errors do you see?

I'll write more later gotta get on a plane soon. but the file layout here works perfectly well. heck it even goes into the arsd folder if you git clone it! (which is why I have locally and why I'm not willing to do the ridiculous nonsense of repeating the directory name). dub is ridiculous in not being able to handle a perfectly normal and perfectly legal D file layout.

andre2007 commented 5 years ago

@adamdruppe To be guided through the errors, please temporarily change in dub.json line 3 the targetType to "library" and execute dub build

engine.d(859,7): Error: instead of C-style syntax, use D-style bool[16][NUM_BUTTONS] buttonsDown engine.d(860,7): Error: instead of C-style syntax, use D-style bool[16][NUM_BUTTONS] buttonsChecked engine.d(865,6): Error: instead of C-style syntax, use D-style int[LAG_QUEUE_SIZE][16][NUM_BUTTONS] buttonLagRemaining engine.d(868,6): Error: instead of C-style syntax, use D-style int[16][NUM_BUTTONS] buttonLagQueueStart engine.d(869,6): Error: instead of C-style syntax, use D-style int[16][NUM_BUTTONS] buttonLagQueueEnd engine.d(870,6): Error: instead of C-style syntax, use D-style int[16][NUM_BUTTONS] buttonLagQueueLength engine.d(874,7): Error: instead of C-style syntax, use D-style bool[LAG_QUEUE_SIZE][16][NUM_BUTTONS] lagbuttonsDown engine.d(878,6): Error: instead of C-style syntax, use D-style int[16][3] stickX engine.d(879,6): Error: instead of C-style syntax, use D-style int[16][3] stickY engine.d(881,7): Error: instead of C-style syntax, use D-style bool[8] mouseButtonsDown engine.d(882,7): Error: instead of C-style syntax, use D-style bool[8] mouseButtonsChecked htmlwidget.d(1035,37): Error: instead of C-style syntax, use D-style Element[] what screen.d(281,12): Error: instead of C-style syntax, use D-style TTF_Font*[8] fonts screen.d(282,16): Error: instead of C-style syntax, use D-style Image[char[]][8] cache screen.d(471,10): Error: instead of C-style syntax, use D-style float[4] f

After these are fixed new errors will through

audio.d(3,8): Error: module SDL is in file 'sdl\SDL.d' which cannot be read

Then

minigui_addons\terminal_emulator_widget.d(27,8): Error: module terminalemulator is in file 'arsd\terminalemulator.d' which cannot be read

... and others

andre2007 commented 5 years ago

The changes proposed to dub.json will solve all problems! @adamdruppe you won't have any discussions in future again about the file structure;)

Here the changes to enabled cgi, http and terminal: (I have added an empty package.d module to avoid the warning about no source codes for root project)

---
 dub.json  | 28 ++++++++++++++++++++--------
 package.d |  1 +
 2 files changed, 21 insertions(+), 8 deletions(-)
 create mode 100644 package.d

diff --git a/dub.json b/dub.json
index 364dda4..7441070 100644
--- a/dub.json
+++ b/dub.json
@@ -1,9 +1,15 @@
 {
    "name": "arsd-official",
-   "targetType": "none",
-   "sourcePaths": ["."],
+   "targetType": "library",
+   "importPaths": ["."],
+   "sourceFiles": ["package.d"],
    "description": "A container of various subpackages that do lots of different things. You should use one of the subpackages instead of the main package in most cases, but you can try the complete package if you get duplicated dependency issues.",
    "license":"BSL-1.0",
+   "dependencies": {
+       ":cgi": "*",
+       ":http": "*",
+       ":terminal": "*"
+   },
    "subPackages": [
        {
            "name": "simpledisplay",
@@ -87,8 +93,10 @@
        {
            "name": "cgi",
            "description": "web server library with cgi, fastcgi, scgi, and embedded http server support",
-           "targetType": "sourceLibrary",
-           "sourceFiles": ["cgi.d"]
+           "targetType": "library",
+           "sourceFiles": ["cgi.d"],
+           "importPaths": ["."],
+           "dflags": ["-mv=arsd.cgi=cgi.d"]
        },
        {
            "name": "mysql",
@@ -132,8 +140,10 @@
            "description": "HTTP client library. Depends on OpenSSL right now on all platforms. On 32 bit Windows, you may need to get OMF openssl lib and dlls (ask me if you can't find it)",
            "libs": ["crypto", "ssl"],
            "versions": ["with_openssl"],
-           "targetType": "sourceLibrary",
-           "sourceFiles": ["http2.d"]
+           "targetType": "library",
+           "sourceFiles": ["http2.d"],
+           "importPaths": ["."],
+           "dflags": ["-mv=arsd.http2=http2.d"]
        },
        {
            "name": "jsvar",
@@ -151,8 +161,10 @@
        {
            "name": "terminal",
            "description": "Cross-platform Terminal I/O with color, mouse support, real time input, etc.",
-           "targetType": "sourceLibrary",
-           "sourceFiles": ["terminal.d"]
+           "targetType": "library",
+           "sourceFiles": ["terminal.d"],
+           "importPaths": ["."],
+           "dflags": ["-mv=arsd.terminal=terminal.d"]
        },
        {
            "name": "ttf",
diff --git a/package.d b/package.d
new file mode 100644
index 0000000..f9223ed
--- /dev/null
+++ b/package.d
@@ -0,0 +1 @@
+module arsd;
\ No newline at end of file
-- 
andre2007 commented 5 years ago

PR https://github.com/adamdruppe/arsd/pull/197

adamdruppe commented 5 years ago

On Thu, Jun 13, 2019 at 09:16:12PM -0700, andre2007 wrote:

To be guided through the errors, please temporarily change in dub.json line 3 the targetType to "library"

Oh, the reason I list specific source files in the subpackages is that many of the modules in there are not supported on their own.

audio.d(3,8): Error: module SDL is in file 'sdl\SDL.d' which cannot be read

Like this, of course, has a dependency on SDL. I've basically replaced it though, but I do personally still have a few programs that depend on this stuff I still like playing with, so I don't wanna remove them.

minigui_addons\terminal_emulator_widget.d(27,8): Error: module terminalemulator is in file 'arsd\terminalemulator.d' which cannot be read

And the terminal emulator lives in a different github repo. The addons thing here are optional widgets with extra functionality that need extra code.

You are not meant to compile these unless you specifically want them.

adamdruppe commented 5 years ago

But that PR is interesting.... I'll try it, let's see what happens. Will bump to version 3.0 since that is potentially a fairly major change (I do source library because I say configure stuff with versions - especially cgi.d where you pick the interface via a version switch. Doing type library, it might not allow that any more. lemme work on it a bit

andre2007 commented 5 years ago

sourceLibrary causes the arsd unittests to be run with applications tests. targetType library works fine together with versions. It is used e.g. in vibe-d tls https://github.com/vibe-d/vibe.d/blob/master/tls/dub.sdl#L51

Although vibe-d d is using configurations in addition to switch between botan, openssl and notls.

adamdruppe commented 5 years ago

On Fri, Jun 14, 2019 at 07:13:08AM -0700, andre2007 wrote:

Although vibe-d d is using configurations in addition to switch between botan, openssl and notls.

it is the configuration thing there that i think is required with library though.

adamdruppe commented 5 years ago

so I just updated the thingy and tagged 3.0, it is now on the dub thing. can you guys test it and lemme know if it works better?

andre2007 commented 5 years ago

Great, thanks a lot. Starting next week I will have time to do some deep testing. Local tests on my PC worked fine.

Independent from this pr, configurations might be a good idea to add (openssl, notls). Add the moment you need in any case lib crypto and ssl for subPackage http although you didn't want to use ssl. https://github.com/adamdruppe/arsd/blob/master/dub.json#L160

andre2007 commented 5 years ago

New issue found: arsd-official-3.0.0\arsd-official\cgi.d(6857,7): Warning: else is dangling, add { } after condition

adamdruppe commented 5 years ago

On Sat, Jun 15, 2019 at 06:13:27AM -0700, andre2007 wrote:

arsd-official-3.0.0\arsd-official\cgi.d(6857,7): Warning: else is dangling, add { } after condition

Walter is right about warnings: they are trash. Especially the ones dmd puts out. I have... very rarely seen these actually find real bugs, almost always just stupid crap like this.

ghost commented 5 years ago

@adamdruppe the warning is valid:

https://wikipedia.org/wiki/Dangling_else

The code in question is ambiguous as written:

https://github.com/adamdruppe/arsd/blob/3fa87f08c4240723565d1b5ac108cf89c1a3a155/cgi.d#L6854-L6858

D is not Python, indentation doesnt really mean anything. You need to use braces to convey meaning. Dub is right, need to add braces after condition. Instead of:

if (2 == 2)
   if (3 == 3) 
      writeln("aaaaa");
   else if (4 == 4) 
      writeln("bbbbb");

Do:

if (2 == 2) {
   if (3 == 3) 
      writeln("aaaaa");
   else if (4 == 4) 
      writeln("bbbbb");
}

In general omitting all the braces is what causes these problems in the first place. It might be better to stop doing that.

adamdruppe commented 5 years ago

On Sat, Jun 15, 2019 at 07:41:03AM -0700, Steven Penny wrote:

The code in question is ambiguous as written:

This is well-defined in D and the code works exactly as intended, and exactly the way it looks. Moreover, this would cause a compile error if it worked the other way, because P wouldn't even be in scope for the final check anymore!

But regardless, I'm rewriting that code this weekend anyway (those lines on my computer are currently commented out), so it will probably not be there for long.

ghost commented 5 years ago

@adamdruppe no, its not. This is very clearly a case of dangling else, its a textbook case. If you cant on wont realize that its too bad, as the fix is quite literally 2 characters.

But if you want to doggedly refuse to use braces thats your business. I used to only use braces when needed, but in general it only leads to bugs like this and less readable code. This example:

if (2 == 2)
   if (3 == 3) 
      writeln("aaaaa");
   else if (4 == 4) 
      writeln("bbbbb");

Could be understood as either of these:

if (2 == 2) {
   if (3 == 3) {
      writeln("aaaaa");
   } else if (4 == 4) {
      writeln("bbbbb");
   }
}
if (2 == 2) {
   if (3 == 3) {
      writeln("aaaaa");
   }
} else if (4 == 4) {
   writeln("bbbbb");
}      

This is why its so important to use braces.

adamdruppe commented 5 years ago

On Sat, Jun 15, 2019 at 07:52:30AM -0700, Steven Penny wrote:

But if you want to doggedly refuse to use braces thats your business.

I usually do, actually. Notice how this is ONE case over the over 100,000 lines in this repo. Clearly, I am doggedly refusing to use braces. lol

0xEAB commented 5 years ago

It's just that those warnings don't come from DUB (that doesn't even parse the code), but DMD…

adamdruppe commented 5 years ago

This whole thing is a pointless diversion; it doesn't really matter anyway. I'm rewriting it today.

(if you're curious what it does, this is a web api dispatcher, and this specific section is trying to handle overloaded functions. I just wrote that section last week, and I'm not happy with it; in practice, it frequently ended up doing the wrong thing and prohibited me from doing what I really wanted to do.

Just what's actually stalling me is my new version has O(n^2) compile time complexity.... O(log n) runtime, which is acceptable, but we'll never get to run time with unacceptably slow builds. I'm a little annoyed at the current 2.2 second build of the project using this, and it is still pretty small. I can just imagine how awful it will get as it reaches completion.)

andre2007 commented 5 years ago

I did some tests and it works fine. Even the issue https://github.com/adamdruppe/arsd/issues/196 is solved due to the switch to targetType library. In theory it is now even possible to add a dub dependency to dub package arsd-official instead if the specific sub package. But there is 1 annoyance. subPackage http says that library crypto and ssl has to be available. Solution would be to add 2 configurations, 1 for the ssl case and 1 for the none ssl case. Dub treats the first configuration as default one. User can overwrite the default in there dub.json using subConfigurations.

andre2007 commented 5 years ago

With arsd-official@3.0.1 the syntax warnings are solved. I think this issue can be closed.

@adamdruppe What is your opinion regarding adding arsd-official to the list of official tested dub packages (https://buildkite.com/dlang/dub/builds/552)? Benefit is, you get a test pipeline for arsd-official (The sub packages referenced as dependencies in dub.json will be tested). Consequence is, all syntax warnings are immediately reported to you and needs to be fixed (subPackages referenced as dependency of root dub package)

adamdruppe commented 5 years ago

On Mon, Jun 17, 2019 at 09:33:42PM -0700, andre2007 wrote:

@adamdruppe What is your opinion regarding adding arsd-official to the list of official tested dub packages (https://buildkite.com/dlang/dub/builds/552)?

I'd be ok with it, though I might have to version out any interactive "unittests"...

andre2007 commented 5 years ago

That would be really great. Just let me know when you are ready, I would then contact @wilzbach at Slack to have arsd-official included in the test pipeline.

Although I am not sure wheter the pipeline just executes dub build, dub test or both.

adamdruppe commented 5 years ago

On Tue, Jun 18, 2019 at 04:50:01PM -0700, Steven Penny wrote:

I dont think this should be closed as I am getting new errors: lld-link: error: could not open crypto.lib: no such file or directory lld-link: error: could not open ssl.lib: no such file or directory

That's just your box missing openssl.

I'd like to offer a schannel alternative for Windows but I haven't gotten around to coding it.

ghost commented 5 years ago

I dont mind using OpenSSL, but I need static linking. Is it possible?

adamdruppe commented 5 years ago

On Tue, Jun 18, 2019 at 06:13:43PM -0700, Steven Penny wrote:

I dont mind using OpenSSL, but I need static linking. Is it possible?

https://duckduckgo.com/?q=openssl+static+library+windows&t=ffab&ia=web

looks like yes.

ghost commented 5 years ago

@adamdruppe sorry I was not clear, I am interested in a prebuilt static library that I can download. In addition it would need to work with D, not C or C++.

Sorry I assumed that would be obvious but hopefully its clear now.

adamdruppe commented 5 years ago

On Tue, Jun 18, 2019 at 06:17:58PM -0700, Steven Penny wrote:

@adamdruppe sorry I was not clear, I am interested in a prebuilt static library that I can download.

One of the results of that web search included that.

In addition it would need to work with D, not C or C++.

They're exactly the same thing.

ghost commented 5 years ago

One of the results of that web search included that.

perhaps you could link to it directly? I didnt see it.

They're exactly the same thing.

No, no they are not:

https://wiki.dlang.org/Curl_on_Windows

adamdruppe commented 5 years ago

This random link has binary downloads of dlls and static libs:

https://github.com/David-Reguera-Garcia-Dreg/Precompiled-OpenSSL-Windows/tree/master/openssl-1.1.0e-vs2013/lib64

The MT versions are static (or there's a 32 bit thing too up one level). The openssl version didn't match what I have on my Linux box, so I added some version stuff to http2.d just now to switch between them. Try -version=newer_openssl to use that.

Note those openssl libs also depend on crypt32.lib advapi32.lib user32.lib, which come with Windows, but you'll need to mention them on your build command to avoid linker errors. You might want to use -m64 or -m32mscoff when building too.

But then, it'll build a static exe.... a 1.7 MB exe... but yeah it works.

andre2007 commented 5 years ago

@adamdruppe Could you add this to dub.json subPackage http

"configurations": [{
        "name": "openssl",
        "libs": [
            "crypto",
            "ssl"
        ],
        "versions": [
            "with_openssl"
        ]
    }, {
        "name": "notls",
        "versions": [
            "without_openssl"
        ]
    }
]

As config openssl is the first config, it is still the default. But now customers can also switch to notls using a subConfiguration in their dub.json. You could also think about setting notls as default configuration, depending what you like more to be the default.

adamdruppe commented 5 years ago

On Wed, Jun 19, 2019 at 04:28:00PM -0700, Steven Penny wrote:

@adamdruppe did you even test those?

You need to use the dmd arguments -m64 for the 64 bit version or -m32mscoff for the 32 bit version.

Not sure where youre getting that information. Those files dont exist with stock Windows 7, 8.1 or 10, so this information isnt really helpful. Perhaps you can link to where these files can be found?

They're part of the standard API bindings. You don't need to find them, they're bundled with compilers and it will just work if you list them on the build command.

ghost commented 5 years ago

Nope

$ dmd -m64 arsd-master/http2.d crypto.lib ssl.lib
lld-link: error: subsystem must be defined
Error: linker exited with status 1
adamdruppe commented 5 years ago

On Wed, Jun 19, 2019 at 06:00:44PM -0700, Steven Penny wrote:

Nope

That command line would never succeed. There is no main() function and no -c switch.

ghost commented 5 years ago

Nope

$ cat bb.d
import arsd.http2, std.stdio;
void main() {
   writeln(3);
}

$ dmd -m64 bb.d arsd-master/http2.d crypto.lib ssl.lib
lld-link: error: undefined symbol: SSL_library_init
adamdruppe commented 5 years ago

On Wed, Jun 19, 2019 at 06:08:40PM -0700, Steven Penny wrote:

Nope

The openssl version didn't match what I have on my Linux box, so I added some version stuff to http2.d just now to switch between them. Try -version=newer_openssl to use that.

make sure you have the http2.d from ~master

ghost commented 5 years ago

Nope

$ dmd -m64 -version=newer_openssl bb.d arsd-master/http2.d ssl.lib crypto.lib
lld-link: error: undefined symbol: __chkstk

side note: this issue is now the most commented issue in the 8 year history of this repo, by a factor of 2. instead of dragging this out forever, if you have some combination of files and commands to get this to work, can you just please share it now so we can end this?

if not, can you please stop posting until then, as i am quite tired of the trial and error.

adamdruppe commented 5 years ago

On Wed, Jun 19, 2019 at 06:36:59PM -0700, Steven Penny wrote:

if not, can you please stop posting until then, as i am quite tired of the trial and error.

You're not even using the tips I gave you in last night's comment.

But the chkstk one didn't happen on my box anyway. Might be (actually I think this is likely) different C runtime library versions (notice the other github link had separate directories for different VS versions - I have 2013 on my box so I used that), or some other thing along those lines.

But chkstk is a C function.

https://docs.microsoft.com/en-us/windows/desktop/DevNotes/-win32-chkstk

So you might be able to find information online about it.

andre2007 commented 5 years ago

@cup do you need the ssl for your scenario?

ghost commented 5 years ago

@andre2007 my main program actually only needs HTTP. However I have some smaller programs that access the GitHub API, which is HTTPS only:

https://developer.github.com/v3#schema

andre2007 commented 5 years ago

There might be a small problem with the arsd http client, in general it should work with ssl. As quick fix there are several options for you

If I find time I will also try to get arsd http client running with ssl and create a pr.

adamdruppe commented 5 years ago

On Fri, Jun 21, 2019 at 10:43:08AM -0700, andre2007 wrote:

There might be a small problem with the arsd http client, in general it should work with ssl. As quick fix there are several options for you

It works fine, I use it every day.

It even works with static openssl on my two Windows computers, with both the Microsoft linker and the LLVM linker. (though I have a hard time seeing why not just use the dll like most everyone else)

I don't know what the problem is on the cup's box. The latest message references a low-level C runtime function, making me suspect it is a crt version mismatch in the build. Maybe even just another lib that isn't being linked in.

  • use windows API If I find time I will also try to get arsd http client running with ssl and create a pr.

If you could change it to use winhttp or schannel in the Windows backend, that'd probably actually be really useful. Heck, even if it is separate functions that just do a couple common cases, that'd be useful af, then I can see about expanding and integrating it more when I have time.

I just haven't used either api myself and have been busy lately so haven't tried it yet.

adamdruppe commented 5 years ago

You might also be able to just kinda cheat and add extern(C) void chkstk() { asm { naked; int 3; } } to your own main program and compile, I betcha it'd build and might even run.