Skip to content

Conversation

@danilohgds
Copy link

Things to notice:

Replacing the strings in oracledbinstall.js with a FOR LOOP in .bat would cause an issue, so SED was used. This means cygwin installation is needed.

Signed-off-by: Danilo Henrique Garcia da Silva danilo_hgds@hotmail.com

… GITBASH is needed due to a bug with replace in bat.

Signed-off-by: Danilo Henrique Garcia da Silva <danilo_hgds@hotmail.com>
@danilohgds
Copy link
Author

@cjbj I sent off the OCA paper thru mail on friday night, they will probably see it on monday.

@cjbj
Copy link
Contributor

cjbj commented Apr 16, 2018

@danilohgds Excellent. I'll let the OCA team know to look out for it when they do their next batch of processing.

@danilohgds
Copy link
Author

@cjbj I had to resend it on monday, section 7 was missing a X mark...probably they will see it today I guess.

@cjbj
Copy link
Contributor

cjbj commented Apr 20, 2018

Did you accidentally push your own private files to the PR?

Also I heard that OCA form is still causing problems! I keep hassling that team about a redesign to reduce errors.

@danilohgds
Copy link
Author

@cjbj , I just noticed that. I was doing some tests, forgot to switch branches my bad.

The form was missing my house number, it is somewhat messy. I will resend soon, had a tough week.

This reverts commit 3f26a42.
@danilohgds
Copy link
Author

@cjbj commit reverted !, now it only has the bat file.

@cjbj
Copy link
Contributor

cjbj commented May 3, 2018

@danilohgds did you send a new OCA application?

@danilohgds
Copy link
Author

@cjbj many days ago, haven't heard anything since.

@danilohgds
Copy link
Author

danilohgds commented May 15, 2018

@cjbj my OCA got accepted, just got some feedback, it should show up in the contributors list in some days.

@cjbj
Copy link
Contributor

cjbj commented May 15, 2018

@danilohgds congrats! I got the email too but haven't worked down my inbox yet. We'll take a quick look at the MR, maybe suggest one or two tweaks, and then copy the changes to our internal repo ready for the next code push to GitHub - I was thinking of pushing the current 2.3-dev code but not doing a binary release until features have landed and been tested.

@pvenkatraman
Copy link

@danilohgds - looks like you are using UNIX utility SED on Windows. What version of SED you are using and which publisher? Is -i option supported by all versions of SED? Thanks

@danilohgds
Copy link
Author

Hi @pvenkatraman , I`m using cygwin to achieve the SED functionality in windows. Cygwin DLL is 2.10.0.

The original FOR loop with keys would not handle the file correctly in the bat file so I changed it.

@cjbj
Copy link
Contributor

cjbj commented May 24, 2018

@pvenkatraman I know you would like some refactoring to avoid the small amount of duplication, and I'd prefer some alignment with the Linux makefile targets, but I'm OK with us merging this PR as it is so we can get on with the 2.3 release. The code works for people who need it and can be be refactored later if it becomes an issue.

@pvenkatraman
Copy link

@danilohgds instead of using "sed" utility can you use findstr.exe. SED is not expected to be available in all Windows systems and looks like cygwin and MKSNT versions vary in command line options. FINDSTR.EXE is a standard windows utility and is expected to be available on all Windows systems.

@danilohgds
Copy link
Author

I will switch to findstr then, should habe it working till friday I hope.

@cjbj
Copy link
Contributor

cjbj commented May 30, 2018

@danilohgds if you're spending time on it, can you also look at removing the duplication of creating the binary - see how Makefile separates the binary & generic package builds?

@danilohgds
Copy link
Author

danilohgds commented Jun 1, 2018

@cjbj So far I haven't been able to workaround the bug involving the FOR tokens syntax.

In my opinion findstr will not help me because the problem is within the For/Token logic itself..

Set "hostname=%NODE_ORACLEDB_PACKAGE_HOSTNAME%"
Set "pathname=%NODE_ORACLEDB_PACKAGE_URL_PATH%"
set "searchpath=/oracle/node-oracledb/releases/download/"
set "search=github.com"

set oracleInstallFile=./oracledbinstall.js
set tempFile=./oracledbinstall.tmp.js

if exist "%tempFile%" del /f /q "%tempFile%"

for /F delims^=^ eol^= %%a in (%oracleInstallFile%) do (
   set newline=%%a
   echo !newline! >> %tempFile%
)

That loop alone, will cause some lines from the original JS file to go bad, for some reason which I cannot figure out.

From :

if (!proxy.startsWith('http://') && !proxy.startsWith('https://')) {
      proxy = 'https://' + proxy;
    }

to ( note how the entire proxy.startsWith part is gone.. )

  if (proxy) { 
    proxyConfig.useProxy = true; 
    if (//') && //')) { 
      proxy = 'https://' + proxy; 
    } 

Once I figure out what is causing this, I will update the PR.

@cjbj
Copy link
Contributor

cjbj commented Jun 1, 2018

@danilohgds I have a differing opinion to @pvenkatraman on this PR. I'm happy for us to merge your current code: - it works for you, it may be useful to other people, and doesn't impact us (because I build the staging packages on a non-Windows platform). Perhaps if you just remove the duplication that will make @pvenkatraman a bit happier and I can get the PR merged!

@cjbj
Copy link
Contributor

cjbj commented Jun 4, 2018

@danilohgds any news on this?

@danilohgds
Copy link
Author

@cjbj got the dedup removed, but I will wait based on what we discussed about the PR.

@cjbj
Copy link
Contributor

cjbj commented Jun 8, 2018

The 2.3 code base now contains this change. Thank you @danilohgds.

@cjbj cjbj closed this Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants