-
Notifications
You must be signed in to change notification settings - Fork 2
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
Runtime folder structure and default/Resource packaging updates #135
Conversation
…aset dependent packaging and signing.
Confirming we don't need the directory to be backwards compatible - I can make quick fixes in any ported data. |
@Sakelun Please go ahead and make that change (if it was necessary to add that). |
Not sure if this is a bug or an FYI, but my install did not have a resources folder inside of templates/_blank/ and so was throwing an error. I created an empty folder there and then things appear to be running fine so far. |
What url should I type in for something that is in the resources folder? Thanks! |
I will be updating that soon, but for the version you have in your hands this moment it maps the URLs as they were, so |
Thanks. Confirmed this appears to work though I'll continue to test, but resources, logs, data, all seem to be working in the new directories as expected. |
Thanks for catching that. This situation is now handled gracefully whether the folder is there or not. We've also added a README file to the folder describing its purpose that will ensure it remains a part of the source repository. |
@Sakelun Just checking...are you waiting for my approval here? I was under the impression that you were going to add a few more fixes (e.g. "handled gracefully whether the folder is there or not." and "added a README"). Or is that already done and I missed it? |
The fixes are on the branch already. The only item pending is reorganizing the output folder per our meeting. |
@benloh The distributable package output has been updated - please test and let me know if you encounter any issues. |
@Sakelun A few issues came up during testing. 1. 'MEME macOS`Sorry to be so nitpicky, but as of a few years ago the official name is 2. npm ci errorDuring testing, I tried to start with a fresh slate to make sure everything will work. So I:
3. Test Code SigningI also wanted to test the whole process. So in spite of the I'm pretty sure this was working last time I'm not done testing the path issues though. |
@benloh We've discussed these issues offline, to re-cap:
Other alternatives would include trialing placing these resources within the bundle ( |
@jdanish We have a LOT of documentation (especially wiki rewrites) that we need to do to fully test and document this. But if you need to get started Monday, here's the essentials of what you need to know:
|
…d be `npm run package`
@Sakelun Sorry this took so long, but I finally wrote up testing instructions and documented what we did. See #136 for the draft wiki update and testing instructions (at bottom of file). A few things to note:
I think I know what the fixes should be, but I wanted to confirm with you to make sure I was reading things correctly. I'm assuming I need to add this and use
|
Also two issues with the readme:
I can also do it, but wasn't sure if readme was being actively worked on. |
@benloh I have reviewed your testing results and made the following updates:
Additional paths have been added to the output of
Example output from
It's a bit counter-intuitive but the messages are correct; the Electron Packager only seems to allow specification of the output directory where platform-specific builds are placed. The naming of the platform folder is not configurable. The build sequence was renaming the Electron Packager output folder to the friendlier folder name we'd expect as the final step. To make this clearer, the build sequence has been changed to do the friendly platform folder rename immediately after the electron packaging step with a log message. Subsequent actions (the template copy) now refer to the friendly output folder:
That would have been correct to identify the application folder (for signing). The executable path varies per platform so to better handle all of these circumstances and eliminate redundant code, it's been isolated within the configuration returned by
Other Improvements / Fixes
Remaining Tasks
|
Hey @jdanish,
Thanks for pointing that out! A .md extension has been added to the README within the |
@jdanish noted. Actually I think we need to rewrite the the whole section "II. Building and Running the Electron App" section, incorporating the changes from #136. TO DO
|
Hi all - I just noticed in the most recent readme that we are still using ../static/dlc instead of /resources ... can we instead use resources? Or just assume it is in resources and so file.pdf is actually resources/file.pdf and sub/file.pdf is resources/sub/file.pdf? And either way, if there is an http or https we look on the web? As noted above, we do not need the static/dlc for backwards compatibility and so it is a bit confusing. If this is a big change, it can wait since I assume we'll revisit this system with the new admin / resource handling and I believe it'll mostly be our team using this app in the short-term. However, if it is easy, seems worth it? We should also note that sub-folders are also AOK. I can update any help text as needed, though. |
This can be done. It would seem best to go the route of just eliminating the prefix sub-directory within the resource database entirely, so
|
…tic/dlc/" prefix.
Confirming the fix to remove the need for .../static/dlc. You now need /file.pdf (or similar). Ideally, we'd cut the / (so you put file.pdf or folder/file.pdf) but I presume there is some reason to include the / ? Either way, this works better now, thanks! |
I wasn't sure, but I've removed it and done some light testing and it doesn't seem consequential. |
Thanks! Some quick testing shows that the new version continues to work without the / for a file or sub-folder, and also works for urls. I think this version is more intuitive. I'll keep testing but it seems like this works now. |
"Is this an issue? Do we need to add support for opening resource links in other windows?" @benloh and @Sakelun good catch! I am using more recent data so didn't notice that, though I've seen it before. For now, no it's not an issue. 95% (or more!) of what we use is local pdfs or html pages (for net logo) and then occasionally google docs. It makes a lot more sense to use those things we can control. If that changes long-term we can put a request in but our focus right now is local materials so no worries. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed works.
This pull request implements the changes requested in #84, namely:
meme
, orblank
)Note:
As part of this update, the
static/dlc
folder is no longer present and has been replaced with a top-levelresources
folder. It is therefore necessary that the Express server remap thestatic/dlc
URL path to this folder to maintain backward compatibility with older datafiles.@benloh has indicated that legacy file support may not be necessary. If so, it would be preferred to map
/
within a datafile to be/resources/
for the purposes of serving content via URL (e.g. https://..../resources/fishtank.pdf) and its location on disk/<meme folder>/resources/
.Test Instructions
Test basic paths
nvm use
npm run clean:all
-- note thedist
andbuilt
folders have been removednpm ci
npm run dev
-- compile to make sure it runshttp://localhost:3000
andhttp://localhost:3000/#/admin
to make sure app loadsnpm run package
boilerplate/dist/MEME macOS
to make sure the electron app has been built correctly:data/
-- should be emptyresources/
-- should be emptytemplates/
-- should have a_blank
template folderLICENSE
LICENSE.chromium.html
meme.app
version
meme.app
locally on your machine -- start the Electron app, and loadhttp://localhost:3000/#/admin
npm run appsign
Test create a blank database with
/templates/_blank
Try different methods of starting a new project from scratch. You should always be able to create a new blank project without any errors.
1. Test
npm run dev
with blank<repodir>/data
anddist
npm run dev
test.loki
file should be created in<repodir>/data/db/test.loki
http://localhost:3000/#/admin
you should see a blank project2. Test
npm run electron
with blank<repodir>/data
anddist
npm run electron
meme.loki
file should be created in<repodir>/data/db/meme.loki
http://localhost:3000/#/admin
you should see a blank project3. Test
npm run app
with blankDelete
<repodir>/data
anddist
Run
npm run app
=> npm should complainRun
npm run package
Run
npm run app
=> npm should complainRun
npm run electron
to build thememe.loki
file.Run
npm run package
to build with the necessary depdencies.Run
npm run appsign
(NOTE you need to have set up code signing -- see [Signing](https://github.com/theRAPTLab/meme/blob/dev-src/file-paths/README-signing.md))A new
meme.loki
file should be created in<repodir>/data/db/meme.loki
An electron app should be built and started
Going to
http://localhost:3000/#/admin
you should see a blank projectTest with default
/templates/test
template folderMake sure that
npm run dev
will generatetest.loki
based on a starting template and that it'll fall back totemplates/_blank
iftemplates/test
is not defined.Test Blank
<repodir>/data
anddist
/<repodir>/templates/test
(if it exists)npm run dev
Test
test
<repodir>/data
anddist
/<repodir>/templates/test
-- use the zip file attached.npm run dev
Test with default
/templates/meme
template folderMake sure the various ways of setting a starting non-blank template work.
Test Blank
<repodir>/data
anddist
/<repodir>/templates/meme
(if it exists)npm run dev
Test
npm run electron
withmeme
<repodir>/data
anddist
/<repodir>/templates/meme
-- use the zip file attached.npm run electron
Test
npm run app
withmeme
<repodir>/data
anddist
/<repodir>/templates/meme
-- use the zip file attached.npm run electron
npm run package
npm run appsign
npm run app
Test
meme.app
withmeme
<repodir>/data
anddist
/<repodir>/templates/meme
-- use the zip file attached.npm run electron
npm run package
npm run appsign
meme.app
to start ElectronTest copy distribution
Make sure it's possible to copy the distribution files to another computer.
Build the electron app (
npm run package
) and copy the/dist/MEME macOS folder
to your new computer/new location, e.g. to~/Desktop/MEME macOS
.Open a terminal.
cd ~/Desktop/MEME macOS
./install.sh
The quarantine flag should now be fixed.
Double click the meme.app Electron application to start the server.
If you see javascript error, then the quarantine flag was probably not removed successfully. Try typing
xattr meme.app
-- you should NOT see the linecom.apple.quarantine
If you copy the app to another computer you may have to run
./install.sh
again.